[rs6000] Fix PR84264: ICE in rs6000_emit_le_vsx_store

Message ID 48c097ef-f09e-c5f5-91fd-05a18e902c0a@bergner.org
State New
Headers show
Series
  • [rs6000] Fix PR84264: ICE in rs6000_emit_le_vsx_store
Related show

Commit Message

Peter Bergner March 4, 2018, 4:55 a.m.
In PR84264, we hit an assert in rs6000_emit_le_vsx_store causing an ICE
in LRA.  We get there, because LRA called the movv4si expander to generate
a spill and the mov pattern calls rs6000_emit_le_vsx_move which in turn
calls rs6000_emit_le_vsx_store.  The rs6000_emit_le_vsx_{load,store}
routines are used at expand time when targeting P8 on an LE system to
generate vsx load/store insns along with their associated byte swaps.
After expand, we shouldn't call them, hence the asserts.

The problem in this case is that LRA calls the movv4si expander to
generate a spill store and we satisfy all the conditions that lead
to calling rs6000_emit_le_vsx_move().  What is different here, is that
with GCC 8, we now generate altivec lvx/stvx insns which are LE friendly
and do not need byte swaps.  In this specific case, LRA is generating
a store to an altivec mem, so we shouldn't call rs6000_emit_le_vsx_move(),
but rather we should just emit the default RTL from the expander.

The simple fix here is to just verify the memory_operand is not an altivec
mem operand before calling rs6000_emit_le_vsx_move.

This passed bootstrap and regtesting on powerpc64le-linux with no
regressions.  Ok for trunk?

Peter


gcc/
	PR target/84264
	* config/rs6000/vector.md:

gcc/testsuite/
	PR target/84264
	* g++.dg/pr84264.C: New test.

Comments

Jakub Jelinek March 4, 2018, 10 a.m. | #1
On Sat, Mar 03, 2018 at 10:55:28PM -0600, Peter Bergner wrote:
> In PR84264, we hit an assert in rs6000_emit_le_vsx_store causing an ICE

> in LRA.  We get there, because LRA called the movv4si expander to generate

> a spill and the mov pattern calls rs6000_emit_le_vsx_move which in turn

> calls rs6000_emit_le_vsx_store.  The rs6000_emit_le_vsx_{load,store}

> routines are used at expand time when targeting P8 on an LE system to

> generate vsx load/store insns along with their associated byte swaps.

> After expand, we shouldn't call them, hence the asserts.

> 

> The problem in this case is that LRA calls the movv4si expander to

> generate a spill store and we satisfy all the conditions that lead

> to calling rs6000_emit_le_vsx_move().  What is different here, is that

> with GCC 8, we now generate altivec lvx/stvx insns which are LE friendly

> and do not need byte swaps.  In this specific case, LRA is generating

> a store to an altivec mem, so we shouldn't call rs6000_emit_le_vsx_move(),

> but rather we should just emit the default RTL from the expander.

> 

> The simple fix here is to just verify the memory_operand is not an altivec

> mem operand before calling rs6000_emit_le_vsx_move.

> 

> This passed bootstrap and regtesting on powerpc64le-linux with no

> regressions.  Ok for trunk?

> 

> Peter

> 

> 

> gcc/

> 	PR target/84264

> 	* config/rs6000/vector.md:


The ChangeLog entry needs fixing.

Otherwise I'll defer to powerpc maintainers.

> 

> gcc/testsuite/

> 	PR target/84264

> 	* g++.dg/pr84264.C: New test.


	Jakub
Peter Bergner March 4, 2018, 3:36 p.m. | #2
On 3/4/18 4:00 AM, Jakub Jelinek wrote:
> On Sat, Mar 03, 2018 at 10:55:28PM -0600, Peter Bergner wrote:

>> gcc/

>> 	PR target/84264

>> 	* config/rs6000/vector.md:

> 

> The ChangeLog entry needs fixing.

> 

> Otherwise I'll defer to powerpc maintainers.


Hmm, too sloopy on my part!  Forgot to write the ChangeLog entry and
sent the email from the wrong account! :-(

Here's the full ChangeLog entry:

gcc/
        PR target/84264
        * config/rs6000/vector.md (mov<mode>): Disallow altivec memory operands.

gcc/testsuite/
        PR target/84264
        * g++.dg/pr84264.C: New test.


Peter
Segher Boessenkool March 5, 2018, 2:55 p.m. | #3
Hi Peter,

On Sat, Mar 03, 2018 at 10:55:28PM -0600, Peter Bergner wrote:
> In PR84264, we hit an assert in rs6000_emit_le_vsx_store causing an ICE

> in LRA.  We get there, because LRA called the movv4si expander to generate

> a spill and the mov pattern calls rs6000_emit_le_vsx_move which in turn

> calls rs6000_emit_le_vsx_store.  The rs6000_emit_le_vsx_{load,store}

> routines are used at expand time when targeting P8 on an LE system to

> generate vsx load/store insns along with their associated byte swaps.

> After expand, we shouldn't call them, hence the asserts.

> 

> The problem in this case is that LRA calls the movv4si expander to

> generate a spill store and we satisfy all the conditions that lead

> to calling rs6000_emit_le_vsx_move().  What is different here, is that

> with GCC 8, we now generate altivec lvx/stvx insns which are LE friendly

> and do not need byte swaps.  In this specific case, LRA is generating

> a store to an altivec mem, so we shouldn't call rs6000_emit_le_vsx_move(),

> but rather we should just emit the default RTL from the expander.

> 

> The simple fix here is to just verify the memory_operand is not an altivec

> mem operand before calling rs6000_emit_le_vsx_move.

> 

> This passed bootstrap and regtesting on powerpc64le-linux with no

> regressions.  Ok for trunk?


Yes this looks correct, okay for trunk.  Thanks.  But it is very
non-obvious; maybe a comment will help, or the code can be restructured
a bit?


Segher
Peter Bergner March 6, 2018, 4:07 p.m. | #4
On 3/5/18 8:55 AM, Segher Boessenkool wrote:
> On Sat, Mar 03, 2018 at 10:55:28PM -0600, Peter Bergner wrote:

>> The simple fix here is to just verify the memory_operand is not an altivec

>> mem operand before calling rs6000_emit_le_vsx_move.

>>

>> This passed bootstrap and regtesting on powerpc64le-linux with no

>> regressions.  Ok for trunk?

> 

> Yes this looks correct, okay for trunk.  Thanks.  But it is very

> non-obvious; maybe a comment will help, or the code can be restructured

> a bit?


As we discussed offline, I committed the patch with an added comment.
Thanks.

Peter

Patch

Index: gcc/config/rs6000/vector.md
===================================================================
--- gcc/config/rs6000/vector.md	(revision 258152)
+++ gcc/config/rs6000/vector.md	(working copy)
@@ -136,8 +136,10 @@  (define_expand "mov<mode>"
       && VECTOR_MEM_VSX_P (<MODE>mode)
       && !TARGET_P9_VECTOR
       && !gpr_or_gpr_p (operands[0], operands[1])
-      && (memory_operand (operands[0], <MODE>mode)
-          ^ memory_operand (operands[1], <MODE>mode)))
+      && ((memory_operand (operands[0], <MODE>mode)
+	   && !altivec_indexed_or_indirect_operand(operands[0], <MODE>mode))
+	  ^ (memory_operand (operands[1], <MODE>mode)
+	     && !altivec_indexed_or_indirect_operand(operands[1], <MODE>mode))))
     {
       rs6000_emit_le_vsx_move (operands[0], operands[1], <MODE>mode);
       DONE;
Index: gcc/testsuite/g++.dg/pr84264.C
===================================================================
--- gcc/testsuite/g++.dg/pr84264.C	(nonexistent)
+++ gcc/testsuite/g++.dg/pr84264.C	(working copy)
@@ -0,0 +1,15 @@ 
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-options "-w -O1 -fstack-protector-strong" } */
+
+void _setjmp ();
+void a (unsigned long *);
+void
+b (void)
+{
+  for (;;)
+    {
+      _setjmp ();
+      unsigned long args[9]{};
+      a (args);
+    }
+}