* [PATCH #2], PR target/81959, Fix ++int to _Float128 conversion on power9
@ 2017-11-30 22:51 Michael Meissner
2017-12-01 23:33 ` Segher Boessenkool
0 siblings, 1 reply; 5+ messages in thread
From: Michael Meissner @ 2017-11-30 22:51 UTC (permalink / raw)
To: GCC Patches, Segher Boessenkool, David Edelsohn, Bill Schmidt
[-- Attachment #1: Type: text/plain, Size: 3723 bytes --]
I submitted the original version of the patch back in August, and then I forgot
about it.
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01600.html
Hi Mike,
On Mon, Aug 28, 2017 at 02:50:02PM -0400, Michael Meissner wrote:
> When I added the optimization for loading 32-bit values directly into the
> vector registers from memory to convert to IEEE 128-bit floating point, I
> forgot to make sure the address did not have PRE_INCREMENT, etc. addressing.
> * config/rs6000/rs6000.md (float_<mode>si2_hw): If register
> allocation hasn't been done, make sure the memory address is
> X-FORM (register+register).
> (floatuns_<mode>si2_hw2): Likewise.
Why is it okay after RA but not before?
Register allocation has fixed the address due to the 'Z' constraint, so it is
no longer an AUTOINC address. I've fixed it so that the function
rs6000_address_for_fpconvert checks whether it is being called after register
allocation, and if so, it does nothing.
> --- gcc/config/rs6000/rs6000.md (revision 251358)
> +++ gcc/config/rs6000/rs6000.md (working copy)
> @@ -14505,6 +14505,9 @@ (define_insn_and_split "float_<mode>si2_
> {
> if (GET_CODE (operands[2]) == SCRATCH)
> operands[2] = gen_reg_rtx (DImode);
> +
> + if (MEM_P (operands[1]) && !reload_completed)
> + operands[1] = rs6000_address_for_fpconvert (operands[1]);
> })
It will need a comment here, then (other callers of
rs6000_address_for_fpconvert do not test for !reload_completed).
All of the other uses of rs6000_address_for_fpconvert are either in
define_expands or on the first splitter pass, which occurs before register
allocation.
Or maybe the predicate should be stricter in all these cases?
nonimmediate_operand allows a lot ;-)
No, then it tends to generate worse code if it is done before the first split
pass (because it no longer keeps the address together). I've been thinking
that in general, we should replace these calls with a new predicate that before
register allocation allows normal memory addresses, but during/after RA, it
becomes more strict. In my experience, with RELOAD that wasn't feasible, but
LRA can handle it (and RELOAD is no longer an issue).
> --- gcc/testsuite/gcc.target/powerpc/pr81959.c (revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/pr81959.c (revision 0)
> @@ -0,0 +1,25 @@
> +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */
> +/* { dg-require-effective-target powerpc_p9vector_ok } */
> +/* { dg-options "-mpower9-vector -O2 -mfloat128" } */
powerpc*-*-*, or does that not work?
It needs 64-bit because various machine independent parts of the compiler want
to use TImode if there is arithmetic support for KFmode to copy things, and
TImode isn't supported in 32-bit.
The __float128 support is not built if the compiler is a 32-bit compiler (the
enabler for _float128 is in linux64.h)
Here is the current version of the patch. I have done bootstraps and make
check with no regressions. Can I check this into the trunk?
The bug shows up in GCC 7 as well. Assuming it backports cleanly, can I check
this into GCC 7 also?
[gcc]
2017-11-30 Michael Meissner <meissner@linux.vnet.ibm.com>
PR target/81959
* config/rs6000/rs6000.c (rs6000_address_for_fpconvert): Check for
whether we can allocate pseudos before trying to fix an address.
* config/rs6000/rs6000.md (float_<mode>si2_hw): Make sure the
memory address is indexed or indirect.
(floatuns_<mode>si2_hw2): Likewise.
[gcct/testsuite]
2017-11-30 Michael Meissner <meissner@linux.vnet.ibm.com>
PR target/81959
* gcc.target/powerpc/pr81959.c: New test.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797
[-- Attachment #2: pr81959.patch05b --]
[-- Type: text/plain, Size: 2531 bytes --]
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c (revision 255177)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -37897,7 +37897,8 @@ rs6000_address_for_fpconvert (rtx x)
gcc_assert (MEM_P (x));
addr = XEXP (x, 0);
- if (! legitimate_indirect_address_p (addr, reload_completed)
+ if (can_create_pseudo_p ()
+ && ! legitimate_indirect_address_p (addr, reload_completed)
&& ! legitimate_indexed_address_p (addr, reload_completed))
{
if (GET_CODE (addr) == PRE_INC || GET_CODE (addr) == PRE_DEC)
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md (revision 255177)
+++ gcc/config/rs6000/rs6000.md (working copy)
@@ -14636,6 +14636,9 @@ (define_insn_and_split "float_<mode>si2_
{
if (GET_CODE (operands[2]) == SCRATCH)
operands[2] = gen_reg_rtx (DImode);
+
+ if (MEM_P (operands[1]))
+ operands[1] = rs6000_address_for_fpconvert (operands[1]);
})
(define_insn_and_split "float<QHI:mode><IEEE128:mode>2"
@@ -14699,6 +14702,9 @@ (define_insn_and_split "floatuns_<mode>s
{
if (GET_CODE (operands[2]) == SCRATCH)
operands[2] = gen_reg_rtx (DImode);
+
+ if (MEM_P (operands[1]))
+ operands[1] = rs6000_address_for_fpconvert (operands[1]);
})
(define_insn_and_split "floatuns<QHI:mode><IEEE128:mode>2"
Index: gcc/testsuite/gcc.target/powerpc/pr81959.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr81959.c (nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/pr81959.c (working copy)
@@ -0,0 +1,25 @@
+/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-options "-mpower9-vector -O2 -mfloat128" } */
+
+/* PR 81959, the compiler raised on unrecognizable insn message in converting
+ int to __float128, where the int had a PRE_INC in the address. */
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE 1024
+#endif
+
+void
+convert_int_to_float128 (__float128 * __restrict__ p,
+ int * __restrict__ q)
+{
+ unsigned long i;
+
+ for (i = 0; i < ARRAY_SIZE; i++)
+ p[i] = (__float128)q[i];
+}
+
+/* { dg-final { scan-assembler {\mlfiwax\M|\mlxsiwax\M} } } */
+/* { dg-final { scan-assembler {\mxscvsdqp\M} } } */
+/* { dg-final { scan-assembler-not {\mmtvsrd\M} } } */
+/* { dg-final { scan-assembler-not {\mmtvsrw[sz]\M} } } */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH #2], PR target/81959, Fix ++int to _Float128 conversion on power9
2017-11-30 22:51 [PATCH #2], PR target/81959, Fix ++int to _Float128 conversion on power9 Michael Meissner
@ 2017-12-01 23:33 ` Segher Boessenkool
2017-12-01 23:46 ` Michael Meissner
2017-12-04 21:32 ` Michael Meissner
0 siblings, 2 replies; 5+ messages in thread
From: Segher Boessenkool @ 2017-12-01 23:33 UTC (permalink / raw)
To: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt
Hi!
On Thu, Nov 30, 2017 at 04:52:44PM -0500, Michael Meissner wrote:
> No, then it tends to generate worse code if it is done before the first split
> pass (because it no longer keeps the address together). I've been thinking
> that in general, we should replace these calls with a new predicate that before
> register allocation allows normal memory addresses, but during/after RA, it
> becomes more strict. In my experience, with RELOAD that wasn't feasible, but
> LRA can handle it (and RELOAD is no longer an issue).
Can't you use the "strict" arg to legitimate_address_p and friends?
> > --- gcc/testsuite/gcc.target/powerpc/pr81959.c (revision 0)
> > +++ gcc/testsuite/gcc.target/powerpc/pr81959.c (revision 0)
> > @@ -0,0 +1,25 @@
> > +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */
> > +/* { dg-require-effective-target powerpc_p9vector_ok } */
> > +/* { dg-options "-mpower9-vector -O2 -mfloat128" } */
>
> powerpc*-*-*, or does that not work?
>
> It needs 64-bit because various machine independent parts of the compiler want
> to use TImode if there is arithmetic support for KFmode to copy things, and
> TImode isn't supported in 32-bit.
That's what lp64 is for.
> The __float128 support is not built if the compiler is a 32-bit compiler (the
> enabler for _float128 is in linux64.h)
So we need some bugzilla predicate for that really?
Okay for trunk. Further improvements welcome ;-) Thanks!
Segher
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH #2], PR target/81959, Fix ++int to _Float128 conversion on power9
2017-12-01 23:33 ` Segher Boessenkool
@ 2017-12-01 23:46 ` Michael Meissner
2017-12-04 21:32 ` Michael Meissner
1 sibling, 0 replies; 5+ messages in thread
From: Michael Meissner @ 2017-12-01 23:46 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt
On Fri, Dec 01, 2017 at 05:33:39PM -0600, Segher Boessenkool wrote:
> Hi!
>
> On Thu, Nov 30, 2017 at 04:52:44PM -0500, Michael Meissner wrote:
> > No, then it tends to generate worse code if it is done before the first split
> > pass (because it no longer keeps the address together). I've been thinking
> > that in general, we should replace these calls with a new predicate that before
> > register allocation allows normal memory addresses, but during/after RA, it
> > becomes more strict. In my experience, with RELOAD that wasn't feasible, but
> > LRA can handle it (and RELOAD is no longer an issue).
>
> Can't you use the "strict" arg to legitimate_address_p and friends?
Well legitimate_address_p allows various D-form address, pre-inc/pre-dec, etc.
It has no context on what the address is being used for. Secondary reload does
have the context, but I've seen post reload passes redo stuff (and typically
then it has to add more code to match the constraints once again).
> > > --- gcc/testsuite/gcc.target/powerpc/pr81959.c (revision 0)
> > > +++ gcc/testsuite/gcc.target/powerpc/pr81959.c (revision 0)
> > > @@ -0,0 +1,25 @@
> > > +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */
> > > +/* { dg-require-effective-target powerpc_p9vector_ok } */
> > > +/* { dg-options "-mpower9-vector -O2 -mfloat128" } */
> >
> > powerpc*-*-*, or does that not work?
> >
> > It needs 64-bit because various machine independent parts of the compiler want
> > to use TImode if there is arithmetic support for KFmode to copy things, and
> > TImode isn't supported in 32-bit.
>
> That's what lp64 is for.
>
> > The __float128 support is not built if the compiler is a 32-bit compiler (the
> > enabler for _float128 is in linux64.h)
>
> So we need some bugzilla predicate for that really?
Or possibly implement the support in 32-bit compilers (and not break embedded
targets).
> Okay for trunk. Further improvements welcome ;-) Thanks!
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH #2], PR target/81959, Fix ++int to _Float128 conversion on power9
2017-12-01 23:33 ` Segher Boessenkool
2017-12-01 23:46 ` Michael Meissner
@ 2017-12-04 21:32 ` Michael Meissner
2017-12-11 16:10 ` Segher Boessenkool
1 sibling, 1 reply; 5+ messages in thread
From: Michael Meissner @ 2017-12-04 21:32 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt
[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]
On Fri, Dec 01, 2017 at 05:33:39PM -0600, Segher Boessenkool wrote:
> Okay for trunk. Further improvements welcome ;-) Thanks!
Here is the patch for GCC 7 (the bug shows up in GCC 7). It is slightly
different due to the surrounding lines in rs6000.c being different. Is it ok
to apply? There were no regressions in the build.
This patch will not be needed in GCC 6.
[gcc]
2017-12-04 Michael Meissner <meissner@linux.vnet.ibm.com>
Back port from trunk
2017-12-01 Michael Meissner <meissner@linux.vnet.ibm.com>
PR target/81959
* config/rs6000/rs6000.c (rs6000_address_for_fpconvert): Check for
whether we can allocate pseudos before trying to fix an address.
* config/rs6000/rs6000.md (float_<mode>si2_hw): Make sure the
memory address is indexed or indirect.
(floatuns_<mode>si2_hw2): Likewise.
[gcct/testsuite]
2017-12-04 Michael Meissner <meissner@linux.vnet.ibm.com>
Back port from trunk
2017-12-01 Michael Meissner <meissner@linux.vnet.ibm.com>
PR target/81959
* gcc.target/powerpc/pr81959.c: New test.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797
[-- Attachment #2: pr81959.patch06b --]
[-- Type: text/plain, Size: 2507 bytes --]
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c (revision 255341)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -39938,7 +39938,8 @@ rs6000_address_for_fpconvert (rtx x)
gcc_assert (MEM_P (x));
addr = XEXP (x, 0);
- if (! legitimate_indirect_address_p (addr, strict_p)
+ if (can_create_pseudo_p ()
+ && ! legitimate_indirect_address_p (addr, strict_p)
&& ! legitimate_indexed_address_p (addr, strict_p))
{
if (GET_CODE (addr) == PRE_INC || GET_CODE (addr) == PRE_DEC)
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md (revision 255341)
+++ gcc/config/rs6000/rs6000.md (working copy)
@@ -14642,6 +14642,9 @@ (define_insn_and_split "float_<mode>si2_
{
if (GET_CODE (operands[2]) == SCRATCH)
operands[2] = gen_reg_rtx (DImode);
+
+ if (MEM_P (operands[1]))
+ operands[1] = rs6000_address_for_fpconvert (operands[1]);
})
(define_insn_and_split "float<QHI:mode><IEEE128:mode>2"
@@ -14705,6 +14708,9 @@ (define_insn_and_split "floatuns_<mode>s
{
if (GET_CODE (operands[2]) == SCRATCH)
operands[2] = gen_reg_rtx (DImode);
+
+ if (MEM_P (operands[1]))
+ operands[1] = rs6000_address_for_fpconvert (operands[1]);
})
(define_insn_and_split "floatuns<QHI:mode><IEEE128:mode>2"
Index: gcc/testsuite/gcc.target/powerpc/pr81959.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr81959.c (nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/pr81959.c (working copy)
@@ -0,0 +1,25 @@
+/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-options "-mpower9-vector -O2 -mfloat128" } */
+
+/* PR 81959, the compiler raised on unrecognizable insn message in converting
+ int to __float128, where the int had a PRE_INC in the address. */
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE 1024
+#endif
+
+void
+convert_int_to_float128 (__float128 * __restrict__ p,
+ int * __restrict__ q)
+{
+ unsigned long i;
+
+ for (i = 0; i < ARRAY_SIZE; i++)
+ p[i] = (__float128)q[i];
+}
+
+/* { dg-final { scan-assembler {\mlfiwax\M|\mlxsiwax\M} } } */
+/* { dg-final { scan-assembler {\mxscvsdqp\M} } } */
+/* { dg-final { scan-assembler-not {\mmtvsrd\M} } } */
+/* { dg-final { scan-assembler-not {\mmtvsrw[sz]\M} } } */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH #2], PR target/81959, Fix ++int to _Float128 conversion on power9
2017-12-04 21:32 ` Michael Meissner
@ 2017-12-11 16:10 ` Segher Boessenkool
0 siblings, 0 replies; 5+ messages in thread
From: Segher Boessenkool @ 2017-12-11 16:10 UTC (permalink / raw)
To: Michael Meissner, GCC Patches, David Edelsohn, Bill Schmidt
On Mon, Dec 04, 2017 at 04:31:55PM -0500, Michael Meissner wrote:
> On Fri, Dec 01, 2017 at 05:33:39PM -0600, Segher Boessenkool wrote:
> > Okay for trunk. Further improvements welcome ;-) Thanks!
>
> Here is the patch for GCC 7 (the bug shows up in GCC 7). It is slightly
> different due to the surrounding lines in rs6000.c being different. Is it ok
> to apply? There were no regressions in the build.
It looks fine... Okay for 7. Thanks!
Segher
> [gcc]
> 2017-12-04 Michael Meissner <meissner@linux.vnet.ibm.com>
>
> Back port from trunk
> 2017-12-01 Michael Meissner <meissner@linux.vnet.ibm.com>
>
> PR target/81959
> * config/rs6000/rs6000.c (rs6000_address_for_fpconvert): Check for
> whether we can allocate pseudos before trying to fix an address.
> * config/rs6000/rs6000.md (float_<mode>si2_hw): Make sure the
> memory address is indexed or indirect.
> (floatuns_<mode>si2_hw2): Likewise.
>
> [gcct/testsuite]
> 2017-12-04 Michael Meissner <meissner@linux.vnet.ibm.com>
>
> Back port from trunk
> 2017-12-01 Michael Meissner <meissner@linux.vnet.ibm.com>
>
> PR target/81959
> * gcc.target/powerpc/pr81959.c: New test.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-11 16:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-30 22:51 [PATCH #2], PR target/81959, Fix ++int to _Float128 conversion on power9 Michael Meissner
2017-12-01 23:33 ` Segher Boessenkool
2017-12-01 23:46 ` Michael Meissner
2017-12-04 21:32 ` Michael Meissner
2017-12-11 16:10 ` Segher Boessenkool
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).