Re: [PATCH] fortran: Expand ieee_arithmetic module's ieee_value inline [PR106579]

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Aug 15, 2022 at 10:00:02PM +0200, FX wrote:
> I have two questions, on this and the ieee_class patch:
> 
> 
> > +  tree type = TREE_TYPE (arg);
> > +  gcc_assert (TREE_CODE (type) == RECORD_TYPE);
> > +  tree field = NULL_TREE;
> > +  for (tree f = TYPE_FIELDS (type); f != NULL_TREE; f = DECL_CHAIN (f))
> > +    if (TREE_CODE (f) == FIELD_DECL)
> > +      {
> > +	gcc_assert (field == NULL_TREE);
> > +	field = f;
> > +      }
> > +  gcc_assert (field);
> 
> Why looping over fields? The class type is a simple type with only one member (and it should be an integer, we can assert that).

I wanted to make sure it has exactly one field.
The ieee_arithmetic.F90 module in libgfortran surely does that, but I've
been worrying about some user overriding that module with something
different.  At least in the C/C++ FEs we had in the past tons of bugs filed
for when some builtin made some assumptions about some headers and data
types in those and then somebody running a testcase that violated that.
Even failed gcc_assertion isn't the best answer to that, ideally one would
verify that upfront and then either error, sorry or ignore the call (leave
it as is).  In that last case, it might be better to do the check on the
gfortran FE types instead of trees (verify the return type or second
argument type is ieee_class_type derived type with a single integral
(hidden) field).

> > +	case IEEE_POSITIVE_ZERO:
> > +	  /* Make this also the default: label.  */
> > +	  label = gfc_build_label_decl (NULL_TREE);
> > +	  tmp = build_case_label (NULL_TREE, NULL_TREE, label);
> > +	  gfc_add_expr_to_block (&body, tmp);
> > +	  real_from_integer (&real, TYPE_MODE (type), 0, SIGNED);
> > +	  break;
> 
> Do we need a default label? It’s not like this is a more likely case than anything else…

The libgfortran version had default: label:
    switch (type) \
    { \
      case IEEE_SIGNALING_NAN: \
        return __builtin_nans ## SUFFIX (""); \
      case IEEE_QUIET_NAN: \
        return __builtin_nan ## SUFFIX (""); \
      case IEEE_NEGATIVE_INF: \
        return - __builtin_inf ## SUFFIX (); \
      case IEEE_NEGATIVE_NORMAL: \
        return -42; \
      case IEEE_NEGATIVE_DENORMAL: \
        return -(GFC_REAL_ ## TYPE ## _TINY) / 2; \
      case IEEE_NEGATIVE_ZERO: \
        return -(GFC_REAL_ ## TYPE) 0; \
      case IEEE_POSITIVE_ZERO: \
        return 0; \
      case IEEE_POSITIVE_DENORMAL: \
        return (GFC_REAL_ ## TYPE ## _TINY) / 2; \
      case IEEE_POSITIVE_NORMAL: \
        return 42; \
      case IEEE_POSITIVE_INF: \
        return __builtin_inf ## SUFFIX (); \
      default: \
        return 0; \
    } \
and I've tried to traslate that into what it generates.
There is at least the IEEE_OTHER_VALUE (aka 0) value
that isn't covered in the switch, but it is just an integer
under the hood, so it could have any other value.

	Jakub