Re: [PATCH v3 3/4] sim: Add partial support for IEEE 754-2008

Mike Frysinger via Gdb-patches
2022-01-12T10:48:18Z

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

On 11 Jan 2022 22:24, Dragan Mladjenovic wrote:
> --- a/sim/common/sim-fpu.c
> +++ b/sim/common/sim-fpu.c
>
> +INLINE_SIM_FPU (int)
> +sim_fpu_is_un (const sim_fpu *l,
> + const sim_fpu *r)

the arguments fit on one line

> +INLINE_SIM_FPU (int)
> +sim_fpu_is_or (const sim_fpu *l,
> + const sim_fpu *r)

same here

> +INLINE_SIM_FPU (int)
> +sim_fpu_un (int *is, const sim_fpu *l, const sim_fpu *r)
>
> +INLINE_SIM_FPU (int)
> +sim_fpu_or (int *is, const sim_fpu *l, const sim_fpu *r)

sorry, but what are these functions supposed to be doing ?

the sim_fpu_{lt,le,eq,ne,...} binary operations are all fairly obvious in
that they map directly to < <= == != ...

but what do un & or map to ? they clearly aren't "Specific comparison
operators" like the others as they aren't actually testing the value of
the operands other than whether they're NaN's.

> + if (sim_fpu_is_nan (l) || sim_fpu_is_nan (r))
> + {
> + *is = 1;
> + return 0;
> + }

the {} section should be indented one more level

> +INLINE_SIM_FPU(int)
> +sim_fpu_classify (const sim_fpu *f)
> +{
> + switch (f->class)
> + {
> + case sim_fpu_class_snan: return SIM_FPU_IS_SNAN;
> + case sim_fpu_class_qnan: return SIM_FPU_IS_QNAN;
> + case sim_fpu_class_infinity:
> + if (!f->sign)
> + return SIM_FPU_IS_PINF;
> + else
> + return SIM_FPU_IS_NINF;
> + case sim_fpu_class_zero:
> + if (!f->sign)
> + return SIM_FPU_IS_PZERO;
> + else
> + return SIM_FPU_IS_NZERO;
> + case sim_fpu_class_number:
> + if (!f->sign)
> + return SIM_FPU_IS_PNUMBER;
> + else
> + return SIM_FPU_IS_NNUMBER;
> + case sim_fpu_class_denorm:
> + if (!f->sign)
> + return SIM_FPU_IS_PDENORM;
> + else
> + return SIM_FPU_IS_NDENORM;

i think these would be a bit more succinct written as:
return f->sign ? SIM_FPU_IS_NINF : SIM_FPU_IS_PINF;

> /* A number of useful constants */
>
> #if EXTERN_SIM_FPU_P
> +static sim_fpu_mode sim_fpu_current_mode = sim_fpu_ieee754_1985;

this isn't a constant. please don't use global variables to hold state. same
as the earlier patch, this should go into the (new) sim_fpu_state structure.