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

[Mike Frysinger <vapier@gentoo.org>]

On 17 Apr 2021 10:58, Jim Wilson wrote:
> Add F and D instruction support.

substance looks fine, just style nits

tests ?

> +execute_d (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
> +{
> ...
> +  static const int round_modes[] =
> +  {
> +      sim_fpu_round_near, sim_fpu_round_zero,
> +      sim_fpu_round_down, sim_fpu_round_up,
> +      sim_fpu_round_default, sim_fpu_round_default,
> +      sim_fpu_round_default
> +  };

hanging indent should be 2 spaces, not 4

> +      u32 |= (cpu->fpregs[rs1].w[1] & 0x80000000) ^ (cpu->fpregs[rs2].w[1] & 0x80000000);

line is too long -- wrap to 80 cols.  i didn't check every line ... this one
just stood out.  so please give it a double check.

> +      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 ?

> +static sim_cia
> +execute_f (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
> +{
> ...
> +  static const int round_modes[] =
> +  {
> +      sim_fpu_round_near, sim_fpu_round_zero,
> +      sim_fpu_round_down, sim_fpu_round_up,
> +      sim_fpu_round_default, sim_fpu_round_default,
> +      sim_fpu_round_default
> +  };

use 2 space hanging indent

> +    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 ?

> --- a/sim/riscv/sim-main.h
> +++ b/sim/riscv/sim-main.h
>  
> +typedef union FRegisterValue
> +{
> +  uint64_t     v[2];
> +  uint32_t     w[4];
> +
> +  int64_t      V[2];
> +  int32_t      W[4];
> +
> +  float        S[4];
> +  double       D[2];
> +
> +} FRegister;

trim that trailing blank line
-mike