Re: [PATCH #2], make Float128 built-in functions work with -mabi=ieeelongdouble

[Segher Boessenkool]

On Thu, Nov 16, 2017 at 12:54:54PM -0500, David Edelsohn wrote:
> On Thu, Nov 16, 2017 at 12:48 PM, Michael Meissner
>  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

Re: [PATCH #2], make Float128 built-in functions work with -mabi=ieeelongdouble

[David Edelsohn]

On Thu, Nov 16, 2017 at 12:48 PM, Michael Meissner
 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.

Thanks, David

Re: [PATCH #2], make Float128 built-in functions work with -mabi=ieeelongdouble

[Michael Meissner]

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?

2017-11-15  Michael Meissner  

* config/rs6000/rs6000.c (rs6000_expand_builtin): Do not do the
switch statement mapping KF built-ins to TF built-ins if we don't
have the proper ISA 3.0 assembler support.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/rs6000.c
===
--- gcc/config/rs6000/rs6000.c  (revision 254837)
+++ gcc/config/rs6000/rs6000.c  (working copy)
@@ -16690,7 +16690,10 @@ rs6000_expand_builtin (tree exp, rtx tar
  double (KFmode) or long double is IEEE 128-bit (TFmode).  It is simpler if
  we only define one variant of the built-in function, and switch the code
  when defining it, rather than defining two built-ins and using the
- overload table in rs6000-c.c to switch between the two.  */
+ overload table in rs6000-c.c to switch between the two.  If we don't have
+ the proper assembler, don't do this switch because CODE_FOR_*kf* and
+ CODE_FOR_*tf* will be CODE_FOR_nothing.  */
+#ifdef HAVE_AS_POWER9
   if (FLOAT128_IEEE_P (TFmode))
 switch (icode)
   {
@@ -16711,6 +16714,7 @@ rs6000_expand_builtin (tree exp, rtx tar
   case CODE_FOR_xsiexpqpf_kf:  icode = CODE_FOR_xsiexpqpf_tf;  break;
   case CODE_FOR_xststdcqp_kf:  icode = CODE_FOR_xststdcqp_tf;  break;
   }
+#endif
 
   if (TARGET_DEBUG_BUILTIN)
 {

Re: [PATCH #2], make Float128 built-in functions work with -mabi=ieeelongdouble

[Segher Boessenkool]

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?


Segher

Re: [PATCH #2], make Float128 built-in functions work with -mabi=ieeelongdouble

[Michael Meissner]

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.

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?

2017-11-15  Michael Meissner  

* config/rs6000/rs6000.c (rs6000_expand_builtin): Do not use a
switch to map KFmode built-in functions to TFmode.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/rs6000.c
===
--- gcc/config/rs6000/rs6000.c  (revision 254782)
+++ gcc/config/rs6000/rs6000.c  (working copy)
@@ -16786,27 +16786,45 @@ rs6000_expand_builtin (tree exp, rtx tar
  double (KFmode) or long double is IEEE 128-bit (TFmode).  It is simpler if
  we only define one variant of the built-in function, and switch the code
  when defining it, rather than defining two built-ins and using the
- overload table in rs6000-c.c to switch between the two.  */
+ overload table in rs6000-c.c to switch between the two.  On some systems
+ like AIX, the KF/TF mode insns are not generated, and they return
+ CODE_FOR_nothing.  */
   if (FLOAT128_IEEE_P (TFmode))
-switch (icode)
-  {
-  default:
-   break;
+{
+  struct map_f128 {
+   enum insn_code from;/* KFmode insn code that is in the tables.  */
+   enum insn_code to;  /* TFmode insn code to use instead.  */
+  };
+
+  static enum insn_code map_insn_code[NUM_INSN_CODES];
+  static bool first_time = true;
+  const static struct map_f128 map[] = {
+   { CODE_FOR_sqrtkf2_odd, CODE_FOR_sqrttf2_odd },
+   { CODE_FOR_trunckfdf2_odd,  CODE_FOR_trunctfdf2_odd },
+   { CODE_FOR_addkf3_odd,  CODE_FOR_addtf3_odd },
+   { CODE_FOR_subkf3_odd,  CODE_FOR_subtf3_odd },
+   { CODE_FOR_mulkf3_odd,  CODE_FOR_multf3_odd },
+   { CODE_FOR_divkf3_odd,  CODE_FOR_divtf3_odd },
+   { CODE_FOR_fmakf4_odd,  CODE_FOR_fmatf4_odd },
+   { CODE_FOR_xsxexpqp_kf, CODE_FOR_xsxexpqp_tf },
+   { CODE_FOR_xsxsigqp_kf, CODE_FOR_xsxsigqp_tf },
+   { CODE_FOR_xststdcnegqp_kf, CODE_FOR_xststdcnegqp_tf },
+   { CODE_FOR_xsiexpqp_kf, CODE_FOR_xsiexpqp_tf },
+   { CODE_FOR_xsiexpqpf_kf,CODE_FOR_xsiexpqpf_tf },
+   { CODE_FOR_xststdcqp_kf,CODE_FOR_xststdcqp_tf },
+  };
+
+  if (first_time)
+   {
+ first_time = false;
+ gcc_assert ((int)CODE_FOR_nothing == 0);
+ for (i = 0; i < ARRAY_SIZE (map); i++)
+   map_insn_code[(int)map[i].from] = map[i].to;
+   }
 
-  case CODE_FOR_sqrtkf2_odd:   icode = CODE_FOR_sqrttf2_odd;   break;
-  case CODE_FOR_trunckfdf2_odd:icode = CODE_FOR_trunctfdf2_odd; break;
-  case CODE_FOR_addkf3_odd:icode = CODE_FOR_addtf3_odd;
break;
-  case CODE_FOR_subkf3_odd:icode = CODE_FOR_subtf3_odd;
break;
-  case CODE_FOR_mulkf3_odd:icode = CODE_FOR_multf3_odd;
break;
-  case CODE_FOR_divkf3_odd:icode = CODE_FOR_divtf3_odd;
break;
-  case CODE_FOR_fmakf4_odd:icode = CODE_FOR_fmatf4_odd;
break;
-  case CODE_FOR_xsxexpqp_kf:   icode = CODE_FOR_xsxexpqp_tf;   break;
-  case CODE_FOR_xsxsigqp_kf:   icode = CODE_FOR_xsxsigqp_tf;   break;
-  case CODE_FOR_xststdcnegqp_kf:   icode = CODE_FOR_xststdcnegqp_tf; break;
-  case CODE_FOR_xsiexpqp_kf:   icode = CODE_FOR_xsiexpqp_tf;   break;
-  case CODE_FOR_xsiexpqpf_kf:  icode = CODE_FOR_xsiexpqpf_tf;  break;
-  case CODE_FOR_xststdcqp_kf:  icode = CODE_FOR_xststdcqp_tf;  break;
-  }
+  if (map_insn_code[(int)icode] != CODE_FOR_nothing)
+   icode = map_insn_code[(int)icode];
+}
 
   if (TARGET_DEBUG_BUILTIN)
 {