Re: [PATCH 06/24] RISC-V: Add fp support.

[Jim Wilson <jimw@sifive.com>]

On Sun, Apr 18, 2021 at 9:08 PM Mike Frysinger <vapier@gentoo.org> wrote:

> On 17 Apr 2021 10:58, Jim Wilson wrote:
> > Add F and D instruction support.
>
> substance looks fine, just style nits
>

I fixed the formatting issues.

tests ?
>

These are fairly well tested by the gcc testsuite.  Sim tests would be good
also, but I would prefer to handle that with a follow up patch when I have
time to write the tests, or manage to find someone to write the tests for
me.

> > +      switch (sim_fpu_is (&sfa))
> > +     {
> > +     case SIM_FPU_IS_NINF:
> > +       cpu->regs[rd] = 1;
> > +       break;
> > +     case SIM_FPU_IS_NNUMBER:
> > +       cpu->regs[rd] = 1 << 1;
>
> i'm a little surprised all these bits don't have constants for them.  but i
> guess binutils wouldn't normally have broken out the FPU state into
> headers ?
>

newlib and glibc have macros for them, which of course are named
differently as no one ever thought about that, but we can't easily use
either of those here, and the inconsistent naming could be a problem.
binutils has no need to name these bits.  There is no instruction that
takes these bits as input, so they never appear directly in the assembly
syntax.  Of course, names could potentially be useful for people writing
assembly code, but no one has asked for that yet, or volunteered to write
it yet.  We don't have an assembly language header file for people to
include that could include these constants.  We don't have a proper
assembly language manual either.

> > +    case MATCH_FCVT_L_S:
> > +      TRACE_INSN (cpu, "fcvt.l.s %s, %s",
> > +               rd_name, frs1_name);
> > +      cpu->regs[rd] = (int64_t) cpu->fpregs[rs1].S[0];
> > +      goto done;
> > +    case MATCH_FCVT_LU_S:
> > +      TRACE_INSN (cpu, "fcvt.lu.s %s, %s",
> > +               rd_name, frs1_name);
> > +      cpu->regs[rd] = (uint64_t) cpu->fpregs[rs1].S[0];
> > +      goto done;
> > +    case MATCH_FCVT_S_L:
> > +      TRACE_INSN (cpu, "fcvt.s.l %s, %s",
> > +               frd_name, rs1_name);
> > +      cpu->fpregs[rd].S[0] = (float) ((int64_t) cpu->regs[rs1]);
> > +      goto done;
> > +    case MATCH_FCVT_S_LU:
> > +      TRACE_INSN (cpu, "fcvt.s.lu %s, %s",
> > +               frd_name, rs1_name);
> > +      cpu->fpregs[rd].S[0] = (float) cpu->regs[rs1];
>
> these raw casts all feel ... wrong.  are the semantics guaranteed to match
> between whatever the host CPU is (e.g. x86_64) and the target (e.g. riscv)
> ?
> or it just seems to mostly work so we aren't going to squint too hard at
> it ?
>

Yes, this is probably wrong.  The other FP math is probably also wrong.
RISC-V has canonical NaNs like ARM, but I don't see any obvious canonical
NaN support in common/sim-fpu.c.  I know that qemu handles this correctly.
RISC-V has NaN boxing for FP values smaller than the FP register size, i.e.
the upper bits must all be 1.  I don't see the code doing this, but this
would only matter for broken code so it is less important.  Qemu still got
this wrong last time I looked at it, but it has been a while since I looked
at the qemu code.

Maybe this stuff can be handled with a bug report and follow up patches?

Jim