* [PATCH][AArch64] PR target/69161: Don't use special predicate for CCmode comparisons in expressions that require matching modes
@ 2016-01-29 14:27 Kyrill Tkachov
2016-02-04 13:49 ` James Greenhalgh
0 siblings, 1 reply; 3+ messages in thread
From: Kyrill Tkachov @ 2016-01-29 14:27 UTC (permalink / raw)
To: GCC Patches; +Cc: Marcus Shawcroft, Richard Earnshaw, James Greenhalgh
[-- Attachment #1: Type: text/plain, Size: 2067 bytes --]
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.
[-- Attachment #2: aarch64-cc-mode-cstore.patch --]
[-- Type: text/x-patch, Size: 4161 bytes --]
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;
+ }
+}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][AArch64] PR target/69161: Don't use special predicate for CCmode comparisons in expressions that require matching modes
2016-01-29 14:27 [PATCH][AArch64] PR target/69161: Don't use special predicate for CCmode comparisons in expressions that require matching modes Kyrill Tkachov
@ 2016-02-04 13:49 ` James Greenhalgh
2016-02-08 12:25 ` Kyrill Tkachov
0 siblings, 1 reply; 3+ messages in thread
From: James Greenhalgh @ 2016-02-04 13:49 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: GCC Patches, Marcus Shawcroft, Richard Earnshaw
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][AArch64] PR target/69161: Don't use special predicate for CCmode comparisons in expressions that require matching modes
2016-02-04 13:49 ` James Greenhalgh
@ 2016-02-08 12:25 ` Kyrill Tkachov
0 siblings, 0 replies; 3+ messages in thread
From: Kyrill Tkachov @ 2016-02-08 12:25 UTC (permalink / raw)
To: James Greenhalgh; +Cc: GCC Patches, Marcus Shawcroft, Richard Earnshaw
[-- 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;
+ }
+}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-02-08 12:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-29 14:27 [PATCH][AArch64] PR target/69161: Don't use special predicate for CCmode comparisons in expressions that require matching modes Kyrill Tkachov
2016-02-04 13:49 ` James Greenhalgh
2016-02-08 12:25 ` Kyrill Tkachov
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).