Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Segher Boessenkool]

On Tue, Jul 12, 2016 at 03:44:37PM -0400, Michael Meissner wrote:
> > I hope you mean for ppc only, otherwise you're breaking a lot of ports...
> 
> Yes I mean for PowerPC only.  I can change the switches name to:
> 
>   --enable-powerpc-lra
>   --enable-powerpc-float128
> 
> if it would be clearer.

That would be nice I think.  For the PowerPC port it is okay with that;
but I don't think I can approve the configure parts.


Segher

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Michael Meissner]

On Tue, Jul 12, 2016 at 01:21:47PM +0200, Bernd Schmidt wrote:
> On 07/12/2016 12:29 PM, Richard Biener wrote:
> 
> >Instead of adding more configury can we please enable LRA on trunk by default
> >_now_?  Otherwise the amount of testing it recieves won't really increase.
> 
> I hope you mean for ppc only, otherwise you're breaking a lot of ports...

Yes I mean for PowerPC only.  I can change the switches name to:

--enable-powerpc-lra
--enable-powerpc-float128

if it would be clearer.  If you would prefer the switches to be undocumented,
and just private switches, that is fine also.

I view --enable-lra/--enble-powerpc-lra as a temporary switch to allow us to
straddle the issue until the performance blocker (PR 69847) is resolved, and
the PowerPC compiler switches to LRA in GCC 7.  Ideally, it would be nice to
eliminate the support for reload in the PowerPC backend (and hence this
switch).  But until we fully transition to lra, it would be useful to have a
way to switch what the default is without having to edit the sandbox.

Likewise, --enable-float128/--enable-powerpc-float128 is a transition switch.
It would be nice to have __float128 on by default without using the -mfloat128
switch, but we have a lot of work in the library arena before we can do this.

-- 
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

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Bernd Schmidt]

On 07/12/2016 06:31 PM, Segher Boessenkool wrote:


Do you have a testcase?  This sounds like fun :-)


This is bfin-elf, gcc.c-torture/compile/pr59417.c, -O3 -fomit-frame-pointer.


Bernd

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Segher Boessenkool]

On Tue, Jul 12, 2016 at 05:17:04PM +0200, Bernd Schmidt wrote:
> The Blackfin thing happens frequently with -fomit-frame-pointer when we have
> 
> (insn 66 64 70 8 (set (reg:SI 96 [ ivtmp.32 ])
> (reg/f:SI 15 FP)) 19 {*movsi_insn}
> 
> Which LRA transforms to an invalid insn:
> 
> (insn 66 64 70 8 (set (reg:SI 15 FP [orig:96 ivtmp.32 ] [96])
> (plus:SI (reg/f:SI 14 SP)
> (const_int 4 [0x4]))) 50 {addsi3}
>  (expr_list:REG_EQUAL (reg/f:SI 15 FP)
> (nil)))
> 
> Haven't fully debugged it but it looks like an instance of the same 
> problem: not using the correct register numbers in elimination. An FP+FP 
> addition would be fine (which is how I'm guessing we arrived at this 
> pattern), but once you substitute the real register number you get an 
> invalid insn. So LRA is somewhat defective in this area.

Do you have a testcase?  This sounds like fun :-)


Segher

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Bernd Schmidt]

On 07/12/2016 02:48 PM, Bernd Schmidt wrote:

No. You can reproduce issues with Blackfin easily by enabling LRA for
it, and I described the C6X issues when the LRA patches were posted for
review.


That was here:
  https://gcc.gnu.org/ml/gcc-patches/2012-10/msg00235.html

The Blackfin thing happens frequently with -fomit-frame-pointer when we have

(insn 66 64 70 8 (set (reg:SI 96 [ ivtmp.32 ])
(reg/f:SI 15 FP)) 19 {*movsi_insn}

Which LRA transforms to an invalid insn:

(insn 66 64 70 8 (set (reg:SI 15 FP [orig:96 ivtmp.32 ] [96])
(plus:SI (reg/f:SI 14 SP)
(const_int 4 [0x4]))) 50 {addsi3}
 (expr_list:REG_EQUAL (reg/f:SI 15 FP)
(nil)))

Haven't fully debugged it but it looks like an instance of the same 
problem: not using the correct register numbers in elimination. An FP+FP 
addition would be fine (which is how I'm guessing we arrived at this 
pattern), but once you substitute the real register number you get an 
invalid insn. So LRA is somewhat defective in this area.



Bernd

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Segher Boessenkool]

On Tue, Jul 12, 2016 at 02:40:28PM +0200, Bernd Schmidt wrote:
> On 07/12/2016 02:15 PM, Segher Boessenkool wrote:
> >How can LRA ever be made default for all targets without breaking those
> >that do not want to change?
> 
> LRA advocates would have to fix the issues with it that prevent it from 
> being usable on certain targets.

That would be the case if the plan was to remove reload.  But the current
proposal is only to change the default, and the affected targets can easily
implement the hook (to say "no") if LRA won't work for them.


Segher

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Bernd Schmidt]

On 07/12/2016 02:45 PM, Segher Boessenkool wrote:

On Tue, Jul 12, 2016 at 02:40:28PM +0200, Bernd Schmidt wrote:

On 07/12/2016 02:15 PM, Segher Boessenkool wrote:

How can LRA ever be made default for all targets without breaking those
that do not want to change?


LRA advocates would have to fix the issues with it that prevent it from
being usable on certain targets. Based on my initial experiments with
Blackfin, and LRA review comments I had based on c6x requirements, I
suspect register elimination will have to be rewritten to start with.


Do you have PR #s for those issues?


No. You can reproduce issues with Blackfin easily by enabling LRA for 
it, and I described the C6X issues when the LRA patches were posted for 
review.



Bernd

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Segher Boessenkool]

On Tue, Jul 12, 2016 at 02:40:28PM +0200, Bernd Schmidt wrote:
> On 07/12/2016 02:15 PM, Segher Boessenkool wrote:
> >How can LRA ever be made default for all targets without breaking those
> >that do not want to change?
> 
> LRA advocates would have to fix the issues with it that prevent it from 
> being usable on certain targets. Based on my initial experiments with 
> Blackfin, and LRA review comments I had based on c6x requirements, I 
> suspect register elimination will have to be rewritten to start with.

Do you have PR #s for those issues?


Segher

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Bernd Schmidt]

On 07/12/2016 02:15 PM, Segher Boessenkool wrote:

How can LRA ever be made default for all targets without breaking those
that do not want to change?


LRA advocates would have to fix the issues with it that prevent it from 
being usable on certain targets. Based on my initial experiments with 
Blackfin, and LRA review comments I had based on c6x requirements, I 
suspect register elimination will have to be rewritten to start with.



Bernd

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Segher Boessenkool]

On Tue, Jul 12, 2016 at 01:31:35PM +0200, Richard Biener wrote:
> On Tue, Jul 12, 2016 at 1:21 PM, Bernd Schmidt  wrote:
> > On 07/12/2016 12:29 PM, Richard Biener wrote:
> >
> >> Instead of adding more configury can we please enable LRA on trunk by
> >> default
> >> _now_?  Otherwise the amount of testing it recieves won't really increase.
> >
> >
> > I hope you mean for ppc only, otherwise you're breaking a lot of ports...
> 
> Of course.  Simply add Init(1) to rs6000.opt mlra.

This is blocked on PR69847.  The plan is still to enable it this stage 1.

How can LRA ever be made default for all targets without breaking those
that do not want to change?


Segher

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Richard Biener]

On Tue, Jul 12, 2016 at 1:21 PM, Bernd Schmidt  wrote:
> On 07/12/2016 12:29 PM, Richard Biener wrote:
>
>> Instead of adding more configury can we please enable LRA on trunk by
>> default
>> _now_?  Otherwise the amount of testing it recieves won't really increase.
>
>
> I hope you mean for ppc only, otherwise you're breaking a lot of ports...

Of course.  Simply add Init(1) to rs6000.opt mlra.

Richard.



>
> Bernd

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Bernd Schmidt]

On 07/12/2016 12:29 PM, Richard Biener wrote:


Instead of adding more configury can we please enable LRA on trunk by default
_now_?  Otherwise the amount of testing it recieves won't really increase.


I hope you mean for ppc only, otherwise you're breaking a lot of ports...


Bernd

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Segher Boessenkool]

On Tue, Jul 12, 2016 at 12:29:04PM +0200, Richard Biener wrote:
> Instead of adding more configury can we please enable LRA on trunk by default
> _now_?  Otherwise the amount of testing it recieves won't really increase.

I agree we should change default_lra_p to return true instead of false.
However, that won't change what the rs6000 port uses (it has its own
implementation of that hook; the uses can select LRA vs. reload with
-mlra).  Other ports that have LRA selectable are in the same boat.
For ports that always use LRA, everything works no matter what.

For rs6000, we cannot change just yet, the performance regressions are
just too big.


Segher

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Richard Biener]

On Mon, Jul 11, 2016 at 10:07 PM, Michael Meissner
 wrote:
> These configuration switches will allow the PowerPC GCC developers to switch
> defaults in the compiler to debug the code, before making the decision to flip
> the default permanently.  In the future, when the defaults have been changed,
> these configuration options would allow developers to go back to the previous
> versions without modifying the code using the --disable- form.
>
> The first option is --enable-lra, which changes the compiler so that the
> default is to use the LRA register allocator instead of the older RELOAD
> allocator. The PowerPC team would like to switch the default, but there is a
> critical bug in LRA that must be fixed before we can change the default:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69847
>
> The second option is --enable-float128, which changes the compiler so that the
> default for VSX systems is to enable the __float128 keyword to allow users
> access to the IEEE 128-bit floating point implementation without having to use
> the keyword.
>
> Both of these switches are debug switches, and are not meant to be used by
> non-developers.
>
> The --enable-lra swich causes the following tests to fail:
>
> * testsuite/gcc.target/powerpc/bool3-p7.c
> * testsuite/gcc.target/powerpc/bool3-p8.c
>
> See bug 71846 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71846) for more
> details.
>
> The --enable-float128 switch causes libquadmath and libstdc++ to fail because
> we do not yet have enough of the support in the compiler to allow these
> libraries to build.  It is our intention, that we will use the
> --enable-float128 option and work on getting the libraries fixed.  If I build
> just a C compiler and disable building libquadmath, there are no regressions 
> in
> the C tests with __float128 enabled or disabled.
>
> Can I check these options into the trunk as non-default options?

Instead of adding more configury can we please enable LRA on trunk by default
_now_?  Otherwise the amount of testing it recieves won't really increase.

Richard.

>
> --
> 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
>

Re: [PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Michael Meissner]

Sigh, I keep forgetting to attach the patch.

2016-07-11  Michael Meissner  

* doc/install.texi (Configuration): Document PowerPC specific
configuration options --enable-lra and --enable-float128.
* configure.ac: Add --enable-lra and --enable-float128 to turn on
-mlra and -mfloat128 by default in the PowerPC compiler.
* configure: Regenerate.
* config.gcc (powerpc*-*-linux*): Add --enable-lra and
--enable-float128 support.
* config/rs6000/rs6000.c (rs6000_option_override_internal): Add
support for --enable-lra and --enable-float128.

-- 
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/doc/install.texi
===
--- gcc/doc/install.texi(revision 238170)
+++ gcc/doc/install.texi(working copy)
@@ -1661,6 +1661,26 @@ Using the GNU Compiler Collection (GCC)}
 See ``RS/6000 and PowerPC Options'' in the main manual
 @end ifhtml
 
+@item --enable-lra
+This option enables @option{-mlra} by default for powerpc-linux.
+@ifnothtml
+@xref{RS/6000 and PowerPC Options,, RS/6000 and PowerPC Options, gcc,
+Using the GNU Compiler Collection (GCC)},
+@end ifnothtml
+@ifhtml
+See ``RS/6000 and PowerPC Options'' in the main manual
+@end ifhtml
+
+@item --enable-float128
+This option enables @option{-mfloat128} by default for powerpc-linux.
+@ifnothtml
+@xref{RS/6000 and PowerPC Options,, RS/6000 and PowerPC Options, gcc,
+Using the GNU Compiler Collection (GCC)},
+@end ifnothtml
+@ifhtml
+See ``RS/6000 and PowerPC Options'' in the main manual
+@end ifhtml
+
 @item --enable-default-ssp
 Turn on @option{-fstack-protector-strong} by default.
 
Index: gcc/configure
===
--- gcc/configure   (revision 238170)
+++ gcc/configure   (working copy)
@@ -918,6 +918,8 @@ enable_rpath
 with_libiconv_prefix
 enable_sjlj_exceptions
 enable_secureplt
+enable_lra
+enable_float128
 enable_leading_mingw64_underscores
 enable_cld
 enable_frame_pointer
@@ -1630,6 +1632,9 @@ Optional Features:
   --enable-sjlj-exceptions
   arrange to use setjmp/longjmp exception handling
   --enable-secureplt  enable -msecure-plt by default for PowerPC
+  --enable-lraenable -mlra by default for PowerPC
+  --enable-float128   enable -mfloat128 by default for PowerPC on Linux
+  VSX systems
   --enable-leading-mingw64-underscores
   enable leading underscores on 64 bit mingw targets
   --enable-cldenable -mcld by default for 32bit x86
@@ -11984,6 +11989,18 @@ if test "${enable_secureplt+set}" = set;
 fi
 
 
+# Check whether --enable-lra was given.
+if test "${enable_lra+set}" = set; then :
+  enableval=$enable_lra;
+fi
+
+
+# Check whether --enable-float128 was given.
+if test "${enable_float128+set}" = set; then :
+  enableval=$enable_float128;
+fi
+
+
 # Check whether --enable-leading-mingw64-underscores was given.
 if test "${enable_leading_mingw64_underscores+set}" = set; then :
   enableval=$enable_leading_mingw64_underscores;
@@ -18475,7 +18492,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18478 "configure"
+#line 18495 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -18581,7 +18598,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18584 "configure"
+#line 18601 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
Index: gcc/configure.ac
===
--- gcc/configure.ac(revision 238170)
+++ gcc/configure.ac(working copy)
@@ -1798,6 +1798,17 @@ AC_ARG_ENABLE(secureplt,
[enable -msecure-plt by default for PowerPC])],
 [], [])
 
+AC_ARG_ENABLE(lra,
+[AS_HELP_STRING([--enable-lra],
+   [enable -mlra by default for PowerPC])],
+[], [])
+
+AC_ARG_ENABLE(float128,
+[AS_HELP_STRING([--enable-float128],
+   [enable -mfloat128 by default for PowerPC on Linux VSX
+systems])],
+[], [])
+
 AC_ARG_ENABLE(leading-mingw64-underscores,
   AS_HELP_STRING([--enable-leading-mingw64-underscores],
  [enable leading underscores on 64 bit mingw targets]),
Index: gcc/config.gcc
===
--- gcc/config.gcc  (revision 238170)
+++ gcc/config.gcc  (working copy)
@@ -2414,6 +2414,12 @@ powerpc*-*-linux*)
if test x${enable_secureplt} = xyes; then
tm_file="rs6000/secureplt.h ${tm_file}"
fi
+   if test x${enable_lra} = xyes; then
+   tm_defines="${tm_defines} ENABLE_LRA=1"
+   fi
+   if test 

[PATCH], PowerPC support to enable -mlra and/or -mfloat128

[Michael Meissner]

These configuration switches will allow the PowerPC GCC developers to switch
defaults in the compiler to debug the code, before making the decision to flip
the default permanently.  In the future, when the defaults have been changed,
these configuration options would allow developers to go back to the previous
versions without modifying the code using the --disable- form.

The first option is --enable-lra, which changes the compiler so that the
default is to use the LRA register allocator instead of the older RELOAD
allocator. The PowerPC team would like to switch the default, but there is a
critical bug in LRA that must be fixed before we can change the default:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69847

The second option is --enable-float128, which changes the compiler so that the
default for VSX systems is to enable the __float128 keyword to allow users
access to the IEEE 128-bit floating point implementation without having to use
the keyword.

Both of these switches are debug switches, and are not meant to be used by
non-developers.

The --enable-lra swich causes the following tests to fail:

* testsuite/gcc.target/powerpc/bool3-p7.c
* testsuite/gcc.target/powerpc/bool3-p8.c

See bug 71846 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71846) for more
details.

The --enable-float128 switch causes libquadmath and libstdc++ to fail because
we do not yet have enough of the support in the compiler to allow these
libraries to build.  It is our intention, that we will use the
--enable-float128 option and work on getting the libraries fixed.  If I build
just a C compiler and disable building libquadmath, there are no regressions in
the C tests with __float128 enabled or disabled.

Can I check these options into the trunk as non-default options?

-- 
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