diff mbox series

rs6000: Don't ICE when spilling an MMA accumulator

Message ID 6dfd029c-e1f3-cb12-fb2c-7d0c703f43f5@linux.ibm.com
State New
Headers show
Series rs6000: Don't ICE when spilling an MMA accumulator | expand

Commit Message

Peter Bergner Aug. 5, 2020, 7:02 p.m. UTC
The following patch fixes one of the bugs discovered in PR96466, namely
when we spill an accumulator that has a known zero value.  In that case,
LRA would emit a new (set (reg:PXI ...) 0) insn, but it would not use the
mma_xxsetaccz pattern to do it.  The solution is to move the xxsetaccz
instruction into the movpxi pattern and have the xxsetaccz pattern call
the move pattern.

This patch fixes the ICE and is in the middle of regression testing.
Ok for trunk if the testing comes back clean?

This is also broken in GCC 10, so ok there after sitting on trunk for
a day or two with no fallout?

Peter

gcc/
	PR target/96446
	* gcc/config/rs6000/mma.md (*movpxi): Add xxsetaccz generation.
	Disable split for zero constant source operand.
	(mma_xxsetaccz): Change to define_expand.  Call gen_movpxi.

gcc/testsuite/
	PR target/96446
	* gcc.target/powerpc/pr96446.c: New test.

Comments

Peter Bergner Aug. 5, 2020, 8:30 p.m. UTC | #1
On 8/5/20 2:02 PM, Peter Bergner wrote:
> This patch fixes the ICE and is in the middle of regression testing.
> Ok for trunk if the testing comes back clean?

Testing was clean with no regressions.

Peter
Segher Boessenkool Aug. 5, 2020, 11:06 p.m. UTC | #2
Hi!

On Wed, Aug 05, 2020 at 02:02:36PM -0500, Peter Bergner wrote:
> The following patch fixes one of the bugs discovered in PR96466, namely
> when we spill an accumulator that has a known zero value.  In that case,
> LRA would emit a new (set (reg:PXI ...) 0) insn, but it would not use the
> mma_xxsetaccz pattern to do it.  The solution is to move the xxsetaccz
> instruction into the movpxi pattern and have the xxsetaccz pattern call
> the move pattern.

>    "TARGET_MMA
> -   && ((gpc_reg_operand (operands[0], PXImode)
> -	&& !(CONST_INT_P (operands[1]) && INTVAL (operands[1]) == 0))
> +   && (gpc_reg_operand (operands[0], PXImode)
>         || gpc_reg_operand (operands[1], PXImode))"

Much nicer now :-)

> +  "@
> +   #
> +   #
> +   #
> +   xxsetaccz %A0"
> +  "&& reload_completed
> +   && !(fpr_reg_operand (operands[0], PXImode)
> +	&& CONST_INT_P (operands[1])
> +	&& INTVAL (operands[1]) == 0)"

You can just say

   && reload_completed
   && !(fpr_reg_operand (operands[0], PXImode) && operands[1] == const0_rtx)

afaics?

Okay (for trunk, and later 10) with or without such a change.  Thanks!


Segher
Peter Bergner Aug. 6, 2020, 3:29 p.m. UTC | #3
On 8/5/20 6:06 PM, Segher Boessenkool wrote:
> You can just say
> 
>    && reload_completed
>    && !(fpr_reg_operand (operands[0], PXImode) && operands[1] == const0_rtx)
> 
> afaics?

Agreed.  I made that change and retested which was clean.


> Okay (for trunk, and later 10) with or without such a change.  Thanks!

Ok, updated patch pushed to trunk.  I'll push to GCC10 after a day or two.
Thanks!

Peter
Peter Bergner Aug. 8, 2020, 10:02 p.m. UTC | #4
On 8/6/20 10:29 AM, Peter Bergner wrote:
> On 8/5/20 6:06 PM, Segher Boessenkool wrote:
> Ok, updated patch pushed to trunk.  I'll push to GCC10 after a day or two.

And now pushed to GCC 10.

Peter
diff mbox series

Patch

diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md
index 15cacfb7fc1..fcca02bfa9f 100644
--- a/gcc/config/rs6000/mma.md
+++ b/gcc/config/rs6000/mma.md
@@ -328,11 +328,17 @@ 
   [(set (match_operand:PXI 0 "nonimmediate_operand" "=d,m,d,d")
 	(match_operand:PXI 1 "input_operand" "m,d,d,O"))]
   "TARGET_MMA
-   && ((gpc_reg_operand (operands[0], PXImode)
-	&& !(CONST_INT_P (operands[1]) && INTVAL (operands[1]) == 0))
+   && (gpc_reg_operand (operands[0], PXImode)
        || gpc_reg_operand (operands[1], PXImode))"
-  "#"
-  "&& reload_completed"
+  "@
+   #
+   #
+   #
+   xxsetaccz %A0"
+  "&& reload_completed
+   && !(fpr_reg_operand (operands[0], PXImode)
+	&& CONST_INT_P (operands[1])
+	&& INTVAL (operands[1]) == 0)"
   [(const_int 0)]
 {
   rs6000_split_multireg_move (operands[0], operands[1]);
@@ -409,12 +415,14 @@ 
   "<acc> %A0"
   [(set_attr "type" "mma")])
 
-(define_insn "mma_xxsetaccz"
-  [(set (match_operand:PXI 0 "fpr_reg_operand" "=d")
+(define_expand "mma_xxsetaccz"
+  [(set (match_operand:PXI 0 "fpr_reg_operand")
 	(const_int 0))]
   "TARGET_MMA"
-  "xxsetaccz %A0"
-  [(set_attr "type" "mma")])
+{
+  emit_insn (gen_movpxi (operands[0], const0_rtx));
+  DONE;
+})
 
 (define_insn "mma_<vv>"
   [(set (match_operand:PXI 0 "fpr_reg_operand" "=&d")
diff --git a/gcc/testsuite/gcc.target/powerpc/pr96446.c b/gcc/testsuite/gcc.target/powerpc/pr96446.c
new file mode 100644
index 00000000000..2083bf4a76a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr96446.c
@@ -0,0 +1,16 @@ 
+/* PR target/96466 */
+/* { dg-do compile } */
+/* { dg-require-effective-target power10_ok } */
+/* { dg-options "-mdejagnu-cpu=power10 -O2" } */
+
+/* Verify we do not ICE on the following.  */
+
+extern void bar0 (void);
+void
+foo0 (__vector_quad *dst)
+{
+  __vector_quad acc;
+  __builtin_mma_xxsetaccz (&acc);
+  bar0 ();
+  *dst = acc;
+}