Re: [PATCH] rs6000, v2: Fix up __SIZEOF_{FLOAT, IBM}128__ defines [PR99708]

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Mar 09, 2022 at 12:34:06PM -0600, Segher Boessenkool wrote:
> It is a common idiom anyway, much clearer than any macro :-)  (But no
> parens please?  sizeof is an operator, not a function).

Ok, changed in my copy.

> > > > +    { "if",		"ibm128_float_type_node "
> > > > +			"? ibm128_float_type_node "
> > > > +			": long_double" },
> > > 
> > > I don't think that is correct.  If there is no __ibm128, there is no
> > > IFmode either (they are two sides of the same coin).  Using DFmode
> > > instead (which is the only thing that "long double" could be if not
> > > IFmode) will just ICE later, if this is used at all.  So best case this
> > > will confuse the reader.
> > 
> > rs6000_expand_builtin has (but at an incorrect spot) code to remap
> > __builtin_{,un}pack_ibm128 to __builtin_{,un}pack_longdouble under the hood,
> > for 2 reasons:
> > 1) __ibm128 type is only supported when VSX is enabled,
> 
> Ugh, another bug.  *^*ET*(^(*^TE($^TW(*T(W

I agree, but I don't have much energy to fix all the bugs in the backend in one
patch.

Probably it could be fixed with incremental:

--- gcc/config/rs6000/rs6000-builtin.cc.jj	2022-03-09 19:55:31.879255798 +0100
+++ gcc/config/rs6000/rs6000-builtin.cc	2022-03-09 20:10:53.778612345 +0100
@@ -713,9 +713,9 @@ rs6000_init_builtins (void)
      For IEEE 128-bit floating point, always create the type __ieee128.  If the
      user used -mfloat128, rs6000-c.cc will create a define from __float128 to
      __ieee128.  */
-  if (TARGET_FLOAT128_TYPE)
+  if (TARGET_LONG_DOUBLE_128 && (!TARGET_IEEEQUAD || TARGET_FLOAT128_TYPE))
     {
-      if (!TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
+      if (!TARGET_IEEEQUAD)
 	ibm128_float_type_node = long_double_type_node;
       else
 	{
@@ -727,7 +727,12 @@ rs6000_init_builtins (void)
       t = build_qualified_type (ibm128_float_type_node, TYPE_QUAL_CONST);
       lang_hooks.types.register_builtin_type (ibm128_float_type_node,
 					      "__ibm128");
+    }
+  else
+    ibm128_float_type_node = NULL_TREE;
 
+  if (TARGET_FLOAT128_TYPE)
+    {
       if (TARGET_IEEEQUAD && TARGET_LONG_DOUBLE_128)
 	ieee128_float_type_node = long_double_type_node;
       else
@@ -737,7 +742,7 @@ rs6000_init_builtins (void)
 					      "__ieee128");
     }
   else
-    ieee128_float_type_node = ibm128_float_type_node = NULL_TREE;
+    ieee128_float_type_node = NULL_TREE;
 
   /* Vector pair and vector quad support.  */
   vector_pair_type_node = make_node (OPAQUE_TYPE);
@@ -3419,11 +3424,10 @@ rs6000_expand_builtin (tree exp, rtx tar
       return const0_rtx;
     }
 
-  if (bif_is_ibm128 (*bifaddr)
-      && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode)))
+  if (bif_is_ibm128 (*bifaddr) && !ibm128_float_type_node)
     {
-      error ("%qs requires %<__ibm128%> type support or %<long double%> "
-	     "to be IBM 128-bit format", bifaddr->bifname);
+      error ("%qs requires %<__ibm128%> type support",
+	     bifaddr->bifname);
       return const0_rtx;
     }
 

which ought to support __ibm128 as the same thing as long double
if long double is double double, and also as a separate IFmode
double double if -mabi=ieeelongdouble and TARGET_FLOAT128_TYPE
(i.e. both IFmode and KFmode are supported).

> >    seems the intent
> >    was that those 2 builtins can be used interchangeably unless
> >    long double is the same as __ieee128, in which case
> >    __builtin_{,un}pack_longdouble errors and
> >    __builtin_{,un}pack_ibm128 works and returns/takes __ibm128
> 
> Aha.
> 
> But the type "if" always means the same thing, in the builtins language.

True.  Before the patch it means ibm128_float_type_node, which is
a tree connected to __ibm128 type if it exists (but could very well be
equal to long_double_type_node) or long_double_type if it doesn't.
With the patch it means the same thing, except that ibm128_float_type_node
is NULL if __ibm128 isn't supported, and at that point if means
long_double_type_node.  So, on the builtins side, if is the same thing
as before.

> > 2) even with VSX, unless -mlong-double-128 -mabi=ieeelongdouble is in
> >    effect, ibm128_float_type_node has TFmode rather than IFmode,
> >    so we can't use the {,un}packif expanders but need {,un}packtf
> >    instead
> 
> TFmode *is* IFmode, in that case.  TFmode is a workaround for
> shortcomings in the generic parts of GCC.  This almost works as well
> even!  But it is confusing no end.

TFmode isn't IFmode in that case, otherwise there wouldn't be the ICE on
(insn 9 8 10 2 (set (reg:IF 119)
         (unspec:TF [
                (reg:DF 120)
                (reg:DF 122)
            ] UNSPEC_PACK_128BIT)) "prqquu.C":4:32 -1
     (nil))
etc.  IFmode and TFmode clearly are separate modes, which in those
cases have the same REAL_MODE_FORMAT.  So, under the hood they are handled
the same, but still it isn't acceptable in RTL to mix the two modes
in cases where RTL requires the same mode.  Like in the SET_DEST/SET_SRC
case must have the same mode, or operands of most arithmetic ops and the
result have to have the same mode, ...

> > Which means the
> >   ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> > is actually the right thing (except for -mlong-double-64 but the
> > patch will reject those builtins in that case) - if
> > ibm128_float_type_node is NULL but long_double_type_node is TFmode,
> > we want __builtin_{,un}pack_ibm128 to work like
> > __builtin_{,un}pack_longdouble and use long double, if
> > ibm128_float_type_node is non-NULL and long_double_type_node is TFmode,
> > ibm128_float_type_node == long_double_type_node and again we want it to
> > work like __builtin_{,un}pack_longdouble and finally if
> > ibm128_float_type_node is non-NULL and long_double_type_node is KFmode,
> > it will use ibm128_float_type_node with IFmode.
> 
> ibm128_float_type_node should always be defined if we can use double-
> double.  That is the whole point of having __ibm128 and __ieee128 types
> at all: they exist whenever they can, and always mean the same thing.
> We should never (have to) do strange gymnastics to access those types.

With the above incremental patch, perhaps (if it works).
But still we can't make if be NULL if __ibm128 isn't supported, because
we'll ICE already during creation of the builtins.
Perhaps error_mark_node might be a possibility (though very risky), but
because of the builtins generator that isn't an option (it appends
"_type_node" and error_mark_node doesn't end with "_type_node").
Using long double will keep the existing behavior for that case and
we'll reject calls to the builtin.

> > E.g. C++
> > auto foo (void) { return __builtin_pack_ibm128 (100000000.0, 0.0000000001); }
> > double bar (long double x) { return __builtin_unpack_ibm128 (x, 0); }
> > double baz (long double x) { return __builtin_unpack_ibm128 (x, 1); }
> > used to be strangely accepted with -mlong-double-64 but did just a weird
> > thing (now is rejected),
> 
> Neither is correct; it should do the simple and correct thing in both
> cases.  Whatever your long double is: long double has nothing whatsoever
> to do with __ibm128 and __ieee128.

With the above incremental patch I could use there __ibm128 instead of
long double in the testcase (but even if I don't, there is just
an extra long double to __ibm128 conversion which is either nop
conversion or __float128 to __ibm128 conversion).
What I wanted to say is that with e.g. -mlong-double-64 when there
just isn't any double double there is nothing to pack or unpack,
we should reject it rather than doing something weird.

> > +  if (bif_is_ibm128 (*bifaddr)
> > +      && !(ibm128_float_type_node || FLOAT128_2REG_P (TFmode)))
> > +    {
> > +      error ("%qs requires %<__ibm128%> type support or %<long double%> "
> > +	     "to be IBM 128-bit format", bifaddr->bifname);
> > +      return const0_rtx;
> > +    }
> 
> So this part is wrong imo.

The incremental patch tweaks this.

> > +  if (bif_is_ibm128 (*bifaddr) && TARGET_LONG_DOUBLE_128 && !TARGET_IEEEQUAD)
> > +    {
> > +      if (fcode == RS6000_BIF_PACK_IF)
> > +	{
> > +	  icode = CODE_FOR_packtf;
> > +	  fcode = RS6000_BIF_PACK_TF;
> > +	  uns_fcode = (size_t) fcode;
> > +	}
> > +      else if (fcode == RS6000_BIF_UNPACK_IF)
> > +	{
> > +	  icode = CODE_FOR_unpacktf;
> > +	  fcode = RS6000_BIF_UNPACK_TF;
> > +	  uns_fcode = (size_t) fcode;
> > +	}
> > +    }
> 
> And this.

This is really necessary.  I could check
&& TYPE_MODE (ibm128_float_type_node) == TFmode
with the incremental patch if that is better understandable, but still, due
to TFmode != IFmode it is required, and after all, it has been there
before too, just done too late so that it already did a wrong thing before.

	Jakub