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
