diff mbox

[AArch64] PR target/69161: Don't use special predicate for CCmode comparisons in expressions that require matching modes

Message ID 56AB76D6.5020104@foss.arm.com
State New
Headers show

Commit Message

Kyrill Tkachov Jan. 29, 2016, 2:27 p.m. UTC 
Hi all,

In this PR we ICE during combine when trying to propagate a comparison into a vec_duplicate,
that is we end up creating the rtx:
(vec_duplicate:V4SI (eq:CC_NZ (reg:CC_NZ 66 cc)
         (const_int 0 [0])))

The documentation for vec_duplicate says:
"The output vector mode must have the same submodes as the input vector mode or the scalar modes"
So this is invalid RTL, which triggers an assert in simplify-rtx to that effect.

It has been suggested on the PR that this is because we use a special_predicate for
aarch64_comparison_operator which means that it ignores the mode when matching.
This is fine when used in RTXes that don't need it, like if_then_else expressions
but can cause trouble when used in places where the modes do matter, like in
SET operations. In this particular ICE the cause was the conditional store
patterns that could end up matching an intermediate rtx during combine of
(set (reg:SI) (eq:CC_NZ x y)).

The suggested solution is to define a separate predicate with the same
conditions as aarch64_comparison_operator but make it not special, so it gets
automatic mode checks to prevent such a situation.

This patch does that.
Bootstrapped and tested on aarch64-linux-gnu.
SPEC2006 codegen did not change with this patch, so there shouldn't be
any code quality regressions.

Ok for trunk?

Thanks,
Kyrill

2016-01-29  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/69161
     * config/aarch64/predicates.md (aarch64_comparison_operator_mode):
     New predicate.
     (aarch64_comparison_operator): Break overly long line into two.
     (aarch64_comparison_operation): Likewise.
     * config/aarch64/aarch64.md (cstorecc4): Use
     aarch64_comparison_operator_mode instead of
     aarch64_comparison_operator.
     (cstore<mode>4): Likewise.
     (aarch64_cstore<mode>): Likewise.
     (*cstoresi_insn_uxtw): Likewise.
     (cstore<mode>_neg): Likewise.
     (*cstoresi_neg_uxtw): Likewise.

2016-01-29  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/69161
     * gcc.c-torture/compile/pr69161.c: New test.

Comments

James Greenhalgh Feb. 4, 2016, 1:49 p.m. UTC | #1 
On Fri, Jan 29, 2016 at 02:27:34PM +0000, Kyrill Tkachov wrote:
> Hi all,
> 
> In this PR we ICE during combine when trying to propagate a comparison into a vec_duplicate,
> that is we end up creating the rtx:
> (vec_duplicate:V4SI (eq:CC_NZ (reg:CC_NZ 66 cc)
>         (const_int 0 [0])))
> 
> The documentation for vec_duplicate says:
> "The output vector mode must have the same submodes as the input vector mode or the scalar modes"
> So this is invalid RTL, which triggers an assert in simplify-rtx to that effect.
> 
> It has been suggested on the PR that this is because we use a special_predicate for
> aarch64_comparison_operator which means that it ignores the mode when matching.
> This is fine when used in RTXes that don't need it, like if_then_else expressions
> but can cause trouble when used in places where the modes do matter, like in
> SET operations. In this particular ICE the cause was the conditional store
> patterns that could end up matching an intermediate rtx during combine of
> (set (reg:SI) (eq:CC_NZ x y)).
> 
> The suggested solution is to define a separate predicate with the same
> conditions as aarch64_comparison_operator but make it not special, so it gets
> automatic mode checks to prevent such a situation.
> 
> This patch does that.
> Bootstrapped and tested on aarch64-linux-gnu.
> SPEC2006 codegen did not change with this patch, so there shouldn't be
> any code quality regressions.
> 
> Ok for trunk?

It would be good to leave a more detailed comment on
"aarch64_comparison_operator_mode" as to why we need it.

Otherwise, this is OK.

Thanks,
James

> 
> Thanks,
> Kyrill
> 
> 2016-01-29  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/69161
>     * config/aarch64/predicates.md (aarch64_comparison_operator_mode):
>     New predicate.
>     (aarch64_comparison_operator): Break overly long line into two.
>     (aarch64_comparison_operation): Likewise.
>     * config/aarch64/aarch64.md (cstorecc4): Use
>     aarch64_comparison_operator_mode instead of
>     aarch64_comparison_operator.
>     (cstore<mode>4): Likewise.
>     (aarch64_cstore<mode>): Likewise.
>     (*cstoresi_insn_uxtw): Likewise.
>     (cstore<mode>_neg): Likewise.
>     (*cstoresi_neg_uxtw): Likewise.
> 
> 2016-01-29  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/69161
>     * gcc.c-torture/compile/pr69161.c: New test.
diff mbox

Patch

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index f900c12cfb4d108fd4c1671c75b465966befee06..46ca6588c93d793668808fa2e3accaa038ea71d4 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -2957,7 +2957,7 @@  (define_expand "cstore<mode>4"
 
 (define_expand "cstorecc4"
   [(set (match_operand:SI 0 "register_operand")
-       (match_operator 1 "aarch64_comparison_operator"
+       (match_operator 1 "aarch64_comparison_operator_mode"
 	[(match_operand 2 "cc_register")
          (match_operand 3 "const0_operand")]))]
   ""
@@ -2969,7 +2969,7 @@  (define_expand "cstorecc4"
 
 (define_expand "cstore<mode>4"
   [(set (match_operand:SI 0 "register_operand" "")
-	(match_operator:SI 1 "aarch64_comparison_operator"
+	(match_operator:SI 1 "aarch64_comparison_operator_mode"
 	 [(match_operand:GPF 2 "register_operand" "")
 	  (match_operand:GPF 3 "aarch64_fp_compare_operand" "")]))]
   ""
@@ -2982,7 +2982,7 @@  (define_expand "cstore<mode>4"
 
 (define_insn "aarch64_cstore<mode>"
   [(set (match_operand:ALLI 0 "register_operand" "=r")
-	(match_operator:ALLI 1 "aarch64_comparison_operator"
+	(match_operator:ALLI 1 "aarch64_comparison_operator_mode"
 	 [(match_operand 2 "cc_register" "") (const_int 0)]))]
   ""
   "cset\\t%<w>0, %m1"
@@ -3027,7 +3027,7 @@  (define_insn_and_split "*compare_cstore<mode>_insn"
 (define_insn "*cstoresi_insn_uxtw"
   [(set (match_operand:DI 0 "register_operand" "=r")
 	(zero_extend:DI
-	 (match_operator:SI 1 "aarch64_comparison_operator"
+	 (match_operator:SI 1 "aarch64_comparison_operator_mode"
 	  [(match_operand 2 "cc_register" "") (const_int 0)])))]
   ""
   "cset\\t%w0, %m1"
@@ -3036,7 +3036,7 @@  (define_insn "*cstoresi_insn_uxtw"
 
 (define_insn "cstore<mode>_neg"
   [(set (match_operand:ALLI 0 "register_operand" "=r")
-	(neg:ALLI (match_operator:ALLI 1 "aarch64_comparison_operator"
+	(neg:ALLI (match_operator:ALLI 1 "aarch64_comparison_operator_mode"
 		  [(match_operand 2 "cc_register" "") (const_int 0)])))]
   ""
   "csetm\\t%<w>0, %m1"
@@ -3047,7 +3047,7 @@  (define_insn "cstore<mode>_neg"
 (define_insn "*cstoresi_neg_uxtw"
   [(set (match_operand:DI 0 "register_operand" "=r")
 	(zero_extend:DI
-	 (neg:SI (match_operator:SI 1 "aarch64_comparison_operator"
+	 (neg:SI (match_operator:SI 1 "aarch64_comparison_operator_mode"
 		  [(match_operand 2 "cc_register" "") (const_int 0)]))))]
   ""
   "csetm\\t%w0, %m1"
diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index e96dc000bea8470daa187dfd7c44e9c9993dbb0f..04cb695b4f038c9a9be5470598939e1ad20f36f4 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -229,10 +229,17 @@  (define_predicate "aarch64_reg_or_imm"
 
 ;; True for integer comparisons and for FP comparisons other than LTGT or UNEQ.
 (define_special_predicate "aarch64_comparison_operator"
-  (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered,ordered,unlt,unle,unge,ungt"))
+  (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered,
+	       ordered,unlt,unle,unge,ungt"))
+
+;; Same as aarch64_comparison_operator but don't ignore the mode.
+(define_predicate "aarch64_comparison_operator_mode"
+  (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered,
+	       ordered,unlt,unle,unge,ungt"))
 
 (define_special_predicate "aarch64_comparison_operation"
-  (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered,ordered,unlt,unle,unge,ungt")
+  (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered,
+	       ordered,unlt,unle,unge,ungt")
 {
   if (XEXP (op, 1) != const0_rtx)
     return false;
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr69161.c b/gcc/testsuite/gcc.c-torture/compile/pr69161.c
new file mode 100644
index 0000000000000000000000000000000000000000..fdbb63f3335851c2d1537b098f245a9b85ef3695
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr69161.c
@@ -0,0 +1,19 @@ 
+/* PR target/69161.  */
+
+char a;
+int b, c, d, e;
+
+void
+foo (void)
+{
+  int f;
+  for (f = 0; f <= 4; f++)
+    {
+      for (d = 0; d < 20; d++)
+	{
+	  __INTPTR_TYPE__ g = (__INTPTR_TYPE__) & c;
+	  b &= (0 != g) > e;
+	}
+      e &= a;
+    }
+}