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

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

Hi James,

On 04/02/16 13:49, James Greenhalgh wrote:
> 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.

Ok, here's the patch with a comment added.
I'll commit it when the arm version is approved as well.

Thanks,
Kyrill

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


[-- Attachment #2: aarch64-comparison-operator.patch --]
[-- Type: text/x-patch, Size: 4313 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 221f106430ea2c4a44352d937e660d0c1bbe10da..bf2140c5fd9458476d42e49100347dd05e1b21df 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3039,7 +3039,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")]))]
   ""
@@ -3051,7 +3051,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" "")]))]
   ""
@@ -3064,7 +3064,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"
@@ -3109,7 +3109,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"
@@ -3118,7 +3118,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"
@@ -3129,7 +3129,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 e80e40683cada374303ea987017f95654531a032..11868278c3d0a1887c2065568f890c3eb8ff7f0b 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -229,10 +229,19 @@ (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.
+;; RTL SET operations require their operands source and destination have
+;; the same modes, so we can't ignore the modes there.  See PR target/69161.
+(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;
+    }
+}