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

[Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org>]

[-- 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.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]