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
