Hi!
On Thu, Mar 10, 2022 at 10:35:36AM +0100, Jakub Jelinek wrote:
> On Wed, Mar 09, 2022 at 04:57:01PM -0600, Segher Boessenkool wrote:
> > > > 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!
> > >
> > > Ok, here is what I'll test momentarily:
> >
> > Thanks again!
>
> Unfortunately, while regtesting on powerpc64le-linux went fine
> (except for
> l += 2.0;
> in the last testcase should have been
> h += 2.0;
> already fixed in my copy), on powerpc64-linux it regressed (both -m32 and
> -m64) following testcase:
>
> +FAIL: gcc.target/powerpc/convert-fp-128.c (test for excess errors)
<snip>
> The problem is that previously on the pre-VSX -mcpu=
> where we support only TFmode being double double we accepted both:
> typedef float __attribute__((mode(IF))) mode_if;
> typedef float __attribute__((mode(KF))) mode_kf;
There is no KFmode in that case, so the test case is just broken? (It
should not depend on VSX at all, but that has been the situation since
forever).
It is not hard to ICE the compiler with bad mode attributes, this has
nothing to do with IEEE QP or anything. It is comparable to how with
bad inline assembler you can cause ICEs (by giving RA no way out, that
it can see anyway).
> in the testcase. handle_mode_attribute calls
> /* Allow the target a chance to translate MODE into something supported.
> See PR86324. */
> mode = targetm.translate_mode_attribute (mode);
> and the rs6000 hook for it looks like:
> /* Target hook for translate_mode_attribute. */
> static machine_mode
> rs6000_translate_mode_attribute (machine_mode mode)
> {
> if ((FLOAT128_IEEE_P (mode)
> && ieee128_float_type_node == long_double_type_node)
> || (FLOAT128_IBM_P (mode)
> && ibm128_float_type_node == long_double_type_node))
> return COMPLEX_MODE_P (mode) ? E_TCmode : E_TFmode;
> return mode;
> }
Bah. That looks like a workaround for some other bug :-(
> With the v3 patch, ibm128_float_type_node == long_double_type_node
> in that case and IF -> TF translation looks correct to me, under
> the hood it will do the same thing.
We should not use IFmode at all there, because it will *not* do the same
thing: we do not handle IFmode in all cases there, only the few that use
this hook.
This needs to be fixed. That needs fixes in generic code, that still
thinks there is a total order on the floating point modes; but there are
IFmode values that are not representable in KFmode, and KFmode values
that are not representable in IFmode. Pretending they can be ordered
(in any direction) gives problems. This is why we have these
workarounds.
> But the fact that it accepted KFmode before and silently handled
> it like TFmode, where KFmode should be IEEE quad, while TFmode in this
> case is double double looks like a bug to me.
Yes. That is exactly why I did not want KFmode to be handled as the
wrong (but valid) mode: it hides problems, and not in a harmless way,
it makes problems much harder to find.
> So, I think we just need to adjust the testcase.
Hopefully! There may be other things still depending on this
translation, so this does not give me the warm fuzzies :-(
> As mode_kf is only used #ifdef __FLOAT128_TYPE__, I've guarded its
> definition with that condition too.
>
> Thus, here is what I've committed in the end.
>
> Note, really unsure about backports, this patch is quite large and
> changes behavior here and there. Probably easiest would be just
> to revert those __SIZEOF_*128__ rs6000 change on release branches?
Yes.
> Or backport a strictly __SIZEOF_*128__ related change (such as
> use TARGET_FLOAT128_TYPE as the condition on whether to predefine
> those macros or not together with moving __SIZEOF_FLOAT128__ to the other
> function next to __float128)?
And then more projects would need extra checks for broken code, making
this whole __SIZEOF_* thing less useful? Not a fan. Also, this would
be not a real backport at all :-(
Thanks again,
Segher
