On Thu, Nov 16, 2017 at 12:54:54PM -0500, David Edelsohn wrote:
> On Thu, Nov 16, 2017 at 12:48 PM, Michael Meissner
> <meiss...@linux.vnet.ibm.com> wrote:
> > On Thu, Nov 16, 2017 at 04:48:18AM -0600, Segher Boessenkool wrote:
> >> On Wed, Nov 15, 2017 at 04:56:10PM -0500, Michael Meissner wrote:
> >> > David tells me that the patch to enable float128 built-in functions to
> >> > work
> >> > with the -mabi=ieeelongdouble option broke AIX because on AIX, the
> >> > float128
> >> > insns are disabled, and they all become CODE_FOR_nothing. The switch
> >> > statement
> >> > that was added in rs6000.c to map KFmode built-in functions to TFmode
> >> > breaks
> >> > under AIX.
> >>
> >> It also breaks on Linux with older binutils (no HAVE_AS_POWER9 defined).
> >>
> >> > I changed the code to have a separate table, and the first call, I build
> >> > the
> >> > table. If the insn was not generated, it will just be CODE_FOR_nothing,
> >> > and
> >> > the KF->TF mode conversion will not be done.
> >> >
> >> > I have tested this on a little endian power8 system and there were no
> >> > regressions. Once David verifies that it builds on AIX, can I check
> >> > this into
> >> > the trunk?
> >>
> >> I don't like this scheme much (huge table, initialisation at runtime,
> >> etc.),
> >> but okay for trunk, to unbreak things there.
> >>
> >> Some comments on the patch:
> >>
> >> > + if (first_time)
> >> > + {
> >> > + first_time = false;
> >> > + gcc_assert ((int)CODE_FOR_nothing == 0);
> >>
> >> No useless cast please. The whole assert is pretty useless fwiw; just
> >> take it out?
> >>
> >> > + for (i = 0; i < ARRAY_SIZE (map); i++)
> >> > + map_insn_code[(int)map[i].from] = map[i].to;
> >> > + }
> >>
> >> Space after cast.
> >>
> >> Only do this for codes that are *not* CODE_FOR_nothing?
> >
> > I must admit to not liking the code, and it is overly complicated.
> >
> > It occurred to me this morning that a much simpler patch is to just #ifdef
> > out
> > the switch statement if we don't have the proper assembler. I tried this
> > on an
> > old power7 system using the system assembler (which does not support the ISA
> > 3.0 instructions) and it built fine. I think this will work on AIX. David
> > can
> > you check this?
> >
> > I will fire off a build, and if it is successful, can I check this patch
> > instead of the other patch?
>
> This patch will solve the problem.
>
> GCC policy prefers runtime tests over #ifdef, but I agree that the
> runtime approach is overly messy. This seems like a reasonable
> approach to me.
Same here. It's a nice simple patch, and with a comment even :-)
We also have 117 #if.* in rs6000.c already, one more won't hurt.
Segher
