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

On Mon, Mar 07, 2022 at 03:37:18PM -0600, Segher Boessenkool wrote:
> > As mentioned in the PR, right now on powerpc* __SIZEOF_{FLOAT,IBM}128__
> > macros are predefined unconditionally, because {ieee,ibm}128_float_type_node
> > is always non-NULL, doesn't reflect whether __ieee128 or __ibm128 are
> > actually supported or not.
> > 
> > The following patch:
> > 1) makes those {ieee,ibm}128_float_type_node trees NULL if we don't
> >    really support them instead of equal to long_double_type_node
> > 2) adjusts the builtins code to use
> >    ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> >    for the 2 builtins, so that we don't ICE during builtins creation
> >    if ibm128_float_type_node is NULL (using error_mark_node instead of
> >    long_double_type_node sounds more risky to me)
> 
> I feel the opposite way: (potentially) using the wrong thing is just a
> ticking time bomb, never "safer".

Actually, fallback to long_double_type_node is the right thing, see below.

> There needs to be a __SIZEOF_IEEE128__ as well, if you like reality :-)

Ok, added now.

> The basic types, that always exist if they are supported at all, are
> __ibm128 and __ieee128.  Long double picks one of those, or some other
> type; and __float128 is a source of confusion (it tries to make things
> more similar to x86, but things are *not* similar anyway :-( )

Not just x86, there are multiple targets with __float128 support, and
when it works, it behaves the same on all of them.
Mike's PR99708 patch (which unfortunately has been backported to various
branches without resolving these issues first) regressed on powerpc64-linux
+FAIL: gcc.dg/pr83198.c (test for excess errors)
+FAIL: c-c++-common/ubsan/float-cast-overflow-7.c   -O2  (test for excess errors)
+UNRESOLVED: c-c++-common/ubsan/float-cast-overflow-7.c   -O2  compilation failed to produce executable
+FAIL: c-c++-common/ubsan/float-cast-overflow-7.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  (test for excess errors)
+UNRESOLVED: c-c++-common/ubsan/float-cast-overflow-7.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  compilation failed to produce executable
+FAIL: c-c++-common/ubsan/float-cast-overflow-7.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  (test for excess errors)
+UNRESOLVED: c-c++-common/ubsan/float-cast-overflow-7.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  compilation failed to produce executable
Those are all tests that use __float128 if __SIZEOF_FLOAT128__ is defined.

> Tested with which -mcpu=?  You need at least power7 to get __ieee128
> support, but it should be tested *without* that as well.  (The actual
> default for powerpc64-linux is power4 (so not even 970), and for
> powerpc-linux it is 603 or such.)

../configure --enable-languages=c,c++,fortran,objc,obj-c++,lto,go --prefix=/home/jakub/GCC; make -j64 > LOG 2>&1 && make -j64 -k check RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}' > LOGC 2>&1; ../contrib/test_summary > LOGT 2>&1
i.e. the oldest supported one.  On powerpc64le-linux also without any
--with- tuning, so power8.

> > 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
> > 	__SIZEOF_FLOAT128__ here and only if __float128 macro is defined.
> 
> (if and only if?)

Changed.

> > 	* config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
> > 	85 instead of 86.
> 
> This should be auto-generated, or just not exist at all
> ("sizeof type_map / sizeof type_map[0]" in the one place it is used,
> instead -- "one place" is after removing it from the definition of the
> array of course, where it is unneeded :-) )

Unfortunately the generator doesn't use libiberty.h, so I couldn't use
ARRAY_SIZE.  Just used sizeof / sizeof [0].

> >  static
> >  const char *rs6000_type_string (tree type_node)
> >  {
> > +  if (type_node == NULL_TREE)
> > +    return "unknown";
> 
> Please say this is NULL?  If you want to indicate it is problematic, you
> can say "***NULL***" or such?

Done.

> > +    { "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, 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
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
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.

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), with -mlong-double-128 without VSX it used to ICE,
e.g. with
error: unrecognizable insn:
    5 | }
      | ^
(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))
and now works, ditto used to ICE with -mlong-double-128 -mcpu=power8 etc.,
and just worked and keeps working with -mlong-double-128 -mabi=ieeelongdouble.

Ok for trunk if it passes another bootstrap/regtest on powerpc64{,le}-linux?

2022-03-09  Jakub Jelinek  <jakub@redhat.com>

	PR target/99708
	* config/rs6000/rs6000.h (enum rs6000_builtin_type_index): Remove
	RS6000_BTI_ptr_ieee128_float and RS6000_BTI_ptr_ibm128_float.
	(ptr_ieee128_float_type_node, ptr_ibm128_float_type_node): Remove.
	* config/rs6000/rs6000-builtin.cc (rs6000_type_string): Return
	"**NULL**" if type_node is NULL first.  Handle
	ieee128_float_type_node.
	(rs6000_init_builtins): Don't initialize ptr_ieee128_float_type_node
	and ptr_ibm128_float_type_node.  Set ibm128_float_type_node and
	ieee128_float_type_node to NULL rather than long_double_type_node if
	they aren't supported.
	(rs6000_expand_builtin): Error if bif_is_ibm128 and
	!ibm128_float_type_node and !FLOAT128_2REG_P (TFmode).  Remap
	RS6000_BIF_{,UN}PACK_IF to RS6000_BIF_{,UN}PACK_TF much earlier
	and only use bif_is_ibm128 check for it.
	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
	__SIZEOF_FLOAT128__ here and only iff __float128 macro is defined.
	(rs6000_cpu_cpp_builtins): Don't define __SIZEOF_FLOAT128__ here.
	Define __SIZEOF_IBM128__=16 if ieee128_float_type_node is non-NULL.
	Formatting fix.
	* config/rs6000/rs6000-gen-builtins.cc: Document ibm128 attribute.
	(struct attrinfo): Add isibm128 member.
	(TYPE_MAP_SIZE): Remove.
	(type_map): Use [] instead of [TYPE_MAP_SIZE].  For "if" use
	ibm128_float_type_node only if it is non-NULL, otherwise fall back
	to long_double_type_node.  Remove "pif" entry.
	(parse_bif_attrs): Handle ibm128 attribute and print it for debugging.
	(write_decls): Output bif_ibm128_bit and bif_is_ibm128.
	(write_type_node): Use sizeof (type_map) / sizeof (type_map[0])
	instead of TYPE_MAP_SIZE.
	(write_bif_static_init): Handle isibm128.
	* config/rs6000/rs6000-builtins.def: Document ibm128 attribute.
	(__builtin_pack_ibm128, __builtin_unpack_ibm128): Add ibm128
	attribute.

	* gcc.dg/pr99708.c: New test.
	* gcc.target/powerpc/pr99708-2.c: New test.



	Jakub

On Wed, 9 Mar 2022 at 13:27, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Mar 07, 2022 at 03:37:18PM -0600, Segher Boessenkool wrote:
> > >     * config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
> > >     85 instead of 86.
> >
> > This should be auto-generated, or just not exist at all
> > ("sizeof type_map / sizeof type_map[0]" in the one place it is used,
> > instead -- "one place" is after removing it from the definition of the
> > array of course, where it is unneeded :-) )
>
> Unfortunately the generator doesn't use libiberty.h, so I couldn't use
> ARRAY_SIZE.  Just used sizeof / sizeof [0].

FWIW this could be simply std::size(type_map) in C++17.

C++11 allows std::end(type_map) - std::begin(type_map) but that's not
really an improvement.

Those functions are all overloaded for arrays and deduce the array length.

Hi!

On Wed, Mar 09, 2022 at 02:27:19PM +0100, Jakub Jelinek wrote:
> On Mon, Mar 07, 2022 at 03:37:18PM -0600, Segher Boessenkool wrote:
> > > 2) adjusts the builtins code to use
> > >    ibm128_float_type_node ? ibm128_float_type_node : long_double_type_node
> > >    for the 2 builtins, so that we don't ICE during builtins creation
> > >    if ibm128_float_type_node is NULL (using error_mark_node instead of
> > >    long_double_type_node sounds more risky to me)
> > 
> > I feel the opposite way: (potentially) using the wrong thing is just a
> > ticking time bomb, never "safer".
> 
> Actually, fallback to long_double_type_node is the right thing, see below.

I don't see how that ever could be true, but I'll see below perhaps :-)

> > There needs to be a __SIZEOF_IEEE128__ as well, if you like reality :-)
> 
> Ok, added now.

Thanks!

> > The basic types, that always exist if they are supported at all, are
> > __ibm128 and __ieee128.  Long double picks one of those, or some other
> > type; and __float128 is a source of confusion (it tries to make things
> > more similar to x86, but things are *not* similar anyway :-( )
> 
> Not just x86, there are multiple targets with __float128 support, and
> when it works, it behaves the same on all of them.

Unfortunately it isn't documented anywhere *what* it should mean, then.

> Mike's PR99708 patch (which unfortunately has been backported to various
> branches without resolving these issues first) regressed on powerpc64-linux

It supposedly was tested there though?  Sigh.

> > Tested with which -mcpu=?  You need at least power7 to get __ieee128
> > support, but it should be tested *without* that as well.  (The actual
> > default for powerpc64-linux is power4 (so not even 970), and for
> > powerpc-linux it is 603 or such.)
> 
> ../configure --enable-languages=c,c++,fortran,objc,obj-c++,lto,go --prefix=/home/jakub/GCC; make -j64 > LOG 2>&1 && make -j64 -k check RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}' > LOGC 2>&1; ../contrib/test_summary > LOGT 2>&1
> i.e. the oldest supported one.  On powerpc64le-linux also without any
> --with- tuning, so power8.

So power4 for powerpc64-linux, and power8 for powerpc64le-linux.

I ask because many people do use --with-cpu=, and the difference matters
a lot here.

> > > 	* config/rs6000/rs6000-gen-builtins.cc (TYPE_MAP_SIZE): Set to
> > > 	85 instead of 86.
> > 
> > This should be auto-generated, or just not exist at all
> > ("sizeof type_map / sizeof type_map[0]" in the one place it is used,
> > instead -- "one place" is after removing it from the definition of the
> > array of course, where it is unneeded :-) )
> 
> Unfortunately the generator doesn't use libiberty.h, so I couldn't use
> ARRAY_SIZE.  Just used sizeof / sizeof [0].

It is a common idiom anyway, much clearer than any macro :-)  (But no
parens please?  sizeof is an operator, not a function).

> > > +    { "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

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

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

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

> 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 -mlong-double-128 without VSX it used to ICE,
> e.g. with
> error: unrecognizable insn:
>     5 | }
>       | ^
> (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))
> and now works, ditto used to ICE with -mlong-double-128 -mcpu=power8 etc.,
> and just worked and keeps working with -mlong-double-128 -mabi=ieeelongdouble.

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

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

Thank you for dealing with this jumble!


Segher

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

On Wed, Mar 09, 2022 at 08:24:24PM +0100, Jakub Jelinek wrote:
> On Wed, Mar 09, 2022 at 12:34:06PM -0600, Segher Boessenkool wrote:
> > > 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.

Yes, but this is a very fundamental thing that causes a lot of
unnecessary complexity :-(

> 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;
>      }

Something like that yes.  It still is all way more complicated than it
should be.  Not your fault of course.

> 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).

It should support __ibm128 always, and __ieee128 whenever VSX is
supported (it should be whenever VMX is supported, and even *always* as
well imnsho, but I haven't won that fight yet).

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

Ah good.

> > > 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, ...

Yes, for some strange reason that I still do not understand we do not
#define one mode to the other.  Buy you agree TFmode is what IFmode is
otherwise...  This stuff is too complicated to explain :-(

> > > 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).

The way it was *meant to be*.  Not how it was implemented it seems :-(

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

But __ibm128 should *always* be supported, so this is a hypothetical
problem.

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

Ah.  Yet Another bug?  -mlong-double-64 should not mean at all that
double-double does not exist / is not meaningful, just that this isn't
what long double uses.

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

If you are fed up with all this, please commit what you have now (after
testing of course ;-) ), and I'll pick up things myself.  Either way,
thank you for all your work on this!


Segher