* [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
@ 2015-11-26 11:15 Kyrill Tkachov
2015-11-26 13:50 ` Bernd Schmidt
0 siblings, 1 reply; 14+ messages in thread
From: Kyrill Tkachov @ 2015-11-26 11:15 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]
Hi all,
In this PR we have an IF-THEN-JOIN formation i.e. no ELSE block and we have a situation
where the THEN block modifies a register used in emit_b, so emit_b must be emitted before
the THEN block. However the bug in the logic that performs these checks ends up to us
emitting emit_a+then_bb followed by emit_b+else_bb.
The fix is pretty simple and involves emitting emit_b (+ else_bb that is empty in this case)
if modified_a is true, even if emit_a is NULL. If emit_a is NULL noce_emit_bb will handle
it properly and not do anything bad, so we're safe.
Bootstrapped and tested on arm, aarch64, x86_64.
Ok for trunk?
Thanks,
Kyrill
2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization/68506
* ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
first if emit_a exists or then_bb modifies 'b'.
2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization/68506
* gcc.c-torture/execute/pr68506.c: New test.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifcvt-if-then-join.patch --]
[-- Type: text/x-patch; name=ifcvt-if-then-join.patch, Size: 1814 bytes --]
commit 08a371d20793bd57e9a68b85beaf2cab0804ed48
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Tue Nov 24 11:49:30 2015 +0000
PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index af7a3b9..3e3dc8d 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2220,7 +2220,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
}
}
- if (emit_a && modified_in_a)
+ if (emit_a || modified_in_a)
{
modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
if (tmp_b && else_bb)
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68506.c b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
new file mode 100644
index 0000000..15984ed
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
@@ -0,0 +1,63 @@
+/* { dg-options "-fno-builtin-abort" } */
+
+int a, b, m, n, o, p, s, u, i;
+char c, q, y;
+short d;
+unsigned char e;
+static int f, h;
+static short g, r, v;
+unsigned t;
+
+extern void abort ();
+
+int
+fn1 (int p1)
+{
+ return a ? p1 : p1 + a;
+}
+
+unsigned char
+fn2 (unsigned char p1, int p2)
+{
+ return p2 >= 2 ? p1 : p1 >> p2;
+}
+
+static short
+fn3 ()
+{
+ int w, x = 0;
+ for (; p < 31; p++)
+ {
+ s = fn1 (c | ((1 && c) == c));
+ t = fn2 (s, x);
+ c = (unsigned) c > -(unsigned) ((o = (m = d = t) == p) <= 4UL) && n;
+ v = -c;
+ y = 1;
+ for (; y; y++)
+ e = v == 1;
+ d = 0;
+ for (; h != 2;)
+ {
+ for (;;)
+ {
+ if (!m)
+ abort ();
+ r = 7 - f;
+ x = e = i | r;
+ q = u * g;
+ w = b == q;
+ if (w)
+ break;
+ }
+ break;
+ }
+ }
+ return x;
+}
+
+int
+main ()
+{
+ fn3 ();
+ return 0;
+}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 11:15 [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case Kyrill Tkachov
@ 2015-11-26 13:50 ` Bernd Schmidt
2015-11-26 14:00 ` Kyrill Tkachov
0 siblings, 1 reply; 14+ messages in thread
From: Bernd Schmidt @ 2015-11-26 13:50 UTC (permalink / raw)
To: Kyrill Tkachov, GCC Patches
On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index af7a3b9..3e3dc8d 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -2220,7 +2220,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
> }
>
> }
> - if (emit_a && modified_in_a)
> + if (emit_a || modified_in_a)
> {
Having stared at it in the debugger for a while, I think I managed to
convince myself that this is correct. So, OK.
A few other comments. This whole if block is indented too far, please
fix while you're there. Also eliminate the unnecessary blank lines
before closing braces (two instances inside this if block). There are
other formatting errors in this function, but those are best left alone
for now.
> modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
Can this ever be true? We arrange for emit_b to set a new pseudo, don't
we? Are we allowing cases where we copy a pattern that sets more than
one register, and is that safe?
Bernd
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 13:50 ` Bernd Schmidt
@ 2015-11-26 14:00 ` Kyrill Tkachov
2015-11-26 14:27 ` Bernd Schmidt
0 siblings, 1 reply; 14+ messages in thread
From: Kyrill Tkachov @ 2015-11-26 14:00 UTC (permalink / raw)
To: Bernd Schmidt, GCC Patches
On 26/11/15 13:40, Bernd Schmidt wrote:
> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
>> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
>> index af7a3b9..3e3dc8d 100644
>> --- a/gcc/ifcvt.c
>> +++ b/gcc/ifcvt.c
>> @@ -2220,7 +2220,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
>> }
>>
>> }
>> - if (emit_a && modified_in_a)
>> + if (emit_a || modified_in_a)
>> {
> Having stared at it in the debugger for a while, I think I managed to convince myself that this is correct. So, OK.
>
Thanks.
> A few other comments. This whole if block is indented too far, please fix while you're there. Also eliminate the unnecessary blank lines before closing braces (two instances inside this if block). There are other formatting errors in this
> function, but those are best left alone for now.
Ok, I'll fix the indentation in that if-else block
>
>> modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
>
> Can this ever be true? We arrange for emit_b to set a new pseudo, don't we? Are we allowing cases where we copy a pattern that sets more than one register, and is that safe?
>
You're right, this statement always sets modifieb_in_b to false. We reject anything bug single_set insns
by this point in the code. I'll replace that with modified_in_b = false;
Thanks,
Kyrill
>
> Bernd
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 14:00 ` Kyrill Tkachov
@ 2015-11-26 14:27 ` Bernd Schmidt
2015-11-26 14:35 ` Kyrill Tkachov
0 siblings, 1 reply; 14+ messages in thread
From: Bernd Schmidt @ 2015-11-26 14:27 UTC (permalink / raw)
To: Kyrill Tkachov, GCC Patches
On 11/26/2015 02:52 PM, Kyrill Tkachov wrote:
>
> On 26/11/15 13:40, Bernd Schmidt wrote:
>> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
>>> modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a,
>>> emit_b);
>>
>> Can this ever be true? We arrange for emit_b to set a new pseudo,
>> don't we? Are we allowing cases where we copy a pattern that sets more
>> than one register, and is that safe?
>
> You're right, this statement always sets modifieb_in_b to false. We
> reject anything bug single_set insns
> by this point in the code. I'll replace that with modified_in_b = false;
Note that there's a mirrored test for modified_in_a, and both are
already initialized to false. Also - careful with single_set, it can
return true even for multiple sets in case there's a REG_DEAD note on
one of them. You might want to strengthen your tests to also include
!multiple_sets. Then, maybe instead of deleting these tests, turn them
into gcc_checking_asserts.
Bernd
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 14:27 ` Bernd Schmidt
@ 2015-11-26 14:35 ` Kyrill Tkachov
2015-11-26 14:37 ` Bernd Schmidt
2015-11-26 16:49 ` Kyrill Tkachov
0 siblings, 2 replies; 14+ messages in thread
From: Kyrill Tkachov @ 2015-11-26 14:35 UTC (permalink / raw)
To: Bernd Schmidt, GCC Patches
On 26/11/15 14:23, Bernd Schmidt wrote:
> On 11/26/2015 02:52 PM, Kyrill Tkachov wrote:
>>
>> On 26/11/15 13:40, Bernd Schmidt wrote:
>>> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
>>>> modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a,
>>>> emit_b);
>>>
>>> Can this ever be true? We arrange for emit_b to set a new pseudo,
>>> don't we? Are we allowing cases where we copy a pattern that sets more
>>> than one register, and is that safe?
>>
>> You're right, this statement always sets modifieb_in_b to false. We
>> reject anything bug single_set insns
>> by this point in the code. I'll replace that with modified_in_b = false;
>
> Note that there's a mirrored test for modified_in_a, and both are already initialized to false.
Yeah, that can be changed to just false too. I'll do that in the next revision.
> Also - careful with single_set, it can return true even for multiple sets in case there's a REG_DEAD note on one of them. You might want to strengthen your tests to also include !multiple_sets. Then, maybe instead of deleting these tests,
> turn them into gcc_checking_asserts.
>
I see. I think the best place to do that would be in insn_valid_noce_process_p and just get it to return
false if multiple_sets (insn) is true.
Would it be ok if I did that as a separate follow-up patch?
We don't have a testcase where this actually causes trouble and I'd like to keep the fix for
this PR as self-contained as possible.
Thanks,
Kyrill
>
> Bernd
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 14:35 ` Kyrill Tkachov
@ 2015-11-26 14:37 ` Bernd Schmidt
2015-11-26 16:49 ` Kyrill Tkachov
1 sibling, 0 replies; 14+ messages in thread
From: Bernd Schmidt @ 2015-11-26 14:37 UTC (permalink / raw)
To: Kyrill Tkachov, GCC Patches
On 11/26/2015 03:35 PM, Kyrill Tkachov wrote:
> Would it be ok if I did that as a separate follow-up patch?
> We don't have a testcase where this actually causes trouble and I'd like
> to keep the fix for
> this PR as self-contained as possible.
Sure.
Bernd
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 14:35 ` Kyrill Tkachov
2015-11-26 14:37 ` Bernd Schmidt
@ 2015-11-26 16:49 ` Kyrill Tkachov
2015-11-26 16:51 ` Bernd Schmidt
1 sibling, 1 reply; 14+ messages in thread
From: Kyrill Tkachov @ 2015-11-26 16:49 UTC (permalink / raw)
To: Bernd Schmidt, GCC Patches
[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]
On 26/11/15 14:35, Kyrill Tkachov wrote:
>
> On 26/11/15 14:23, Bernd Schmidt wrote:
>> On 11/26/2015 02:52 PM, Kyrill Tkachov wrote:
>>>
>>> On 26/11/15 13:40, Bernd Schmidt wrote:
>>>> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
>>>>> modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a,
>>>>> emit_b);
>>>>
>>>> Can this ever be true? We arrange for emit_b to set a new pseudo,
>>>> don't we? Are we allowing cases where we copy a pattern that sets more
>>>> than one register, and is that safe?
>>>
>>> You're right, this statement always sets modifieb_in_b to false. We
>>> reject anything bug single_set insns
>>> by this point in the code. I'll replace that with modified_in_b = false;
>>
>> Note that there's a mirrored test for modified_in_a, and both are already initialized to false.
>
> Yeah, that can be changed to just false too. I'll do that in the next revision.
>
Here is the updated patch.
I've reindented the if-else blocks and removed the always-false initialisation of modified_a and modified_b.
Re-tested on x86_64, aarch64.
Ok for trunk?
Thanks,
Kyrill
2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization/68506
* ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
first if emit_a exists or then_bb modifies 'b'. Reindent if-else
blocks.
2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization/68506
* gcc.c-torture/execute/pr68506.c: New test.
>> Also - careful with single_set, it can return true even for multiple sets in case there's a REG_DEAD note on one of them. You might want to strengthen your tests to also include !multiple_sets. Then, maybe instead of deleting these
>> tests, turn them into gcc_checking_asserts.
>>
>
> I see. I think the best place to do that would be in insn_valid_noce_process_p and just get it to return
> false if multiple_sets (insn) is true.
>
> Would it be ok if I did that as a separate follow-up patch?
> We don't have a testcase where this actually causes trouble and I'd like to keep the fix for
> this PR as self-contained as possible.
>
> Thanks,
> Kyrill
>
>
>
>>
>> Bernd
>>
>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifcvt-fix.patch --]
[-- Type: text/x-patch; name=ifcvt-fix.patch, Size: 3690 bytes --]
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index af7a3b96f38bea429f86916fc176913cac2e6ebc..9092b377e45082ec07a30d60b070575f4dc68641 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2206,7 +2206,6 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
swap insn that sets up A with the one that sets up B. If even
that doesn't help, punt. */
- modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
if (tmp_b && then_bb)
{
FOR_BB_INSNS (then_bb, tmp_insn)
@@ -2220,40 +2219,37 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
}
}
- if (emit_a && modified_in_a)
- {
- modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
- if (tmp_b && else_bb)
- {
- FOR_BB_INSNS (else_bb, tmp_insn)
- /* Don't check inside insn_b. We will have changed it to emit_b
- with a destination that doesn't conflict. */
- if (!(insn_b && tmp_insn == insn_b)
- && modified_in_p (orig_a, tmp_insn))
- {
- modified_in_b = true;
- break;
- }
-
- }
- if (modified_in_b)
- goto end_seq_and_fail;
-
- if (!noce_emit_bb (emit_b, else_bb, b_simple))
- goto end_seq_and_fail;
+ if (emit_a || modified_in_a)
+ {
+ if (tmp_b && else_bb)
+ {
+ FOR_BB_INSNS (else_bb, tmp_insn)
+ /* Don't check inside insn_b. We will have changed it to emit_b
+ with a destination that doesn't conflict. */
+ if (!(insn_b && tmp_insn == insn_b)
+ && modified_in_p (orig_a, tmp_insn))
+ {
+ modified_in_b = true;
+ break;
+ }
+ }
+ if (modified_in_b)
+ goto end_seq_and_fail;
- if (!noce_emit_bb (emit_a, then_bb, a_simple))
- goto end_seq_and_fail;
- }
- else
- {
- if (!noce_emit_bb (emit_a, then_bb, a_simple))
- goto end_seq_and_fail;
+ if (!noce_emit_bb (emit_b, else_bb, b_simple))
+ goto end_seq_and_fail;
- if (!noce_emit_bb (emit_b, else_bb, b_simple))
- goto end_seq_and_fail;
+ if (!noce_emit_bb (emit_a, then_bb, a_simple))
+ goto end_seq_and_fail;
+ }
+ else
+ {
+ if (!noce_emit_bb (emit_a, then_bb, a_simple))
+ goto end_seq_and_fail;
- }
+ if (!noce_emit_bb (emit_b, else_bb, b_simple))
+ goto end_seq_and_fail;
+ }
target = noce_emit_cmove (if_info, x, code, XEXP (if_info->cond, 0),
XEXP (if_info->cond, 1), a, b);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68506.c b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
new file mode 100644
index 0000000000000000000000000000000000000000..15984edfe0812dc1dbbc8a07bc5b95997ed3acb9
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
@@ -0,0 +1,63 @@
+/* { dg-options "-fno-builtin-abort" } */
+
+int a, b, m, n, o, p, s, u, i;
+char c, q, y;
+short d;
+unsigned char e;
+static int f, h;
+static short g, r, v;
+unsigned t;
+
+extern void abort ();
+
+int
+fn1 (int p1)
+{
+ return a ? p1 : p1 + a;
+}
+
+unsigned char
+fn2 (unsigned char p1, int p2)
+{
+ return p2 >= 2 ? p1 : p1 >> p2;
+}
+
+static short
+fn3 ()
+{
+ int w, x = 0;
+ for (; p < 31; p++)
+ {
+ s = fn1 (c | ((1 && c) == c));
+ t = fn2 (s, x);
+ c = (unsigned) c > -(unsigned) ((o = (m = d = t) == p) <= 4UL) && n;
+ v = -c;
+ y = 1;
+ for (; y; y++)
+ e = v == 1;
+ d = 0;
+ for (; h != 2;)
+ {
+ for (;;)
+ {
+ if (!m)
+ abort ();
+ r = 7 - f;
+ x = e = i | r;
+ q = u * g;
+ w = b == q;
+ if (w)
+ break;
+ }
+ break;
+ }
+ }
+ return x;
+}
+
+int
+main ()
+{
+ fn3 ();
+ return 0;
+}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 16:49 ` Kyrill Tkachov
@ 2015-11-26 16:51 ` Bernd Schmidt
2015-11-26 17:05 ` Kyrill Tkachov
0 siblings, 1 reply; 14+ messages in thread
From: Bernd Schmidt @ 2015-11-26 16:51 UTC (permalink / raw)
To: Kyrill Tkachov, GCC Patches
On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
> that doesn't help, punt. */
>
> - modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
> if (tmp_b && then_bb)
> {
These bits I thought would be part of a followup patch (which would also
guard against single_set problems), and as I mentioned I'd rather have a
checking assert.
So take these deletions out and leave them for the followup, and the
patch is ok everywhere. No need for a full retest given that practically
the same patch has been tested already, just make sure you can build cc1.
Bernd
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 16:51 ` Bernd Schmidt
@ 2015-11-26 17:05 ` Kyrill Tkachov
2015-11-27 9:45 ` Kyrill Tkachov
0 siblings, 1 reply; 14+ messages in thread
From: Kyrill Tkachov @ 2015-11-26 17:05 UTC (permalink / raw)
To: Bernd Schmidt, GCC Patches
On 26/11/15 16:49, Bernd Schmidt wrote:
> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>> that doesn't help, punt. */
>>
>> - modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>> if (tmp_b && then_bb)
>> {
> These bits I thought would be part of a followup patch (which would also guard against single_set problems), and as I mentioned I'd rather have a checking assert.
Yes, you're right. I have the checking_assert statement in the followup that I've been testing.
I'll move the deletion of these two statements there as well to minimise the changes to this patch.
I'll move these bits to that patch, re-build cc1 and commit.
Thanks for your guidance,
Kyrill
> So take these deletions out and leave them for the followup, and the patch is ok everywhere. No need for a full retest given that practically the same patch has been tested already, just make sure you can build cc1.
>
>
> Bernd
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-26 17:05 ` Kyrill Tkachov
@ 2015-11-27 9:45 ` Kyrill Tkachov
2015-11-27 14:09 ` Richard Biener
0 siblings, 1 reply; 14+ messages in thread
From: Kyrill Tkachov @ 2015-11-27 9:45 UTC (permalink / raw)
To: Bernd Schmidt, GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]
On 26/11/15 16:54, Kyrill Tkachov wrote:
>
> On 26/11/15 16:49, Bernd Schmidt wrote:
>> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>>> that doesn't help, punt. */
>>>
>>> - modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>>> if (tmp_b && then_bb)
>>> {
>> These bits I thought would be part of a followup patch (which would also guard against single_set problems), and as I mentioned I'd rather have a checking assert.
> Yes, you're right. I have the checking_assert statement in the followup that I've been testing.
> I'll move the deletion of these two statements there as well to minimise the changes to this patch.
>
> I'll move these bits to that patch, re-build cc1 and commit.
>
Here it is.
I'm committing this to trunk.
Thanks,
Kyrill
2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization/68506
* ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
first if emit_a exists or then_bb modifies 'b'. Reindent if-else
blocks.
2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization/68506
* gcc.c-torture/execute/pr68506.c: New test.
> Thanks for your guidance,
> Kyrill
>
>> So take these deletions out and leave them for the followup, and the patch is ok everywhere. No need for a full retest given that practically the same patch has been tested already, just make sure you can build cc1.
>>
>>
>> Bernd
>>
>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifcvt-fix.patch --]
[-- Type: text/x-patch; name=ifcvt-fix.patch, Size: 3522 bytes --]
commit ba7633ec30e8e25d7dc1975893bf56eadf223404
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Tue Nov 24 11:49:30 2015 +0000
PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index af7a3b9..3ce9fe6 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2220,40 +2220,38 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
}
}
- if (emit_a && modified_in_a)
- {
- modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
- if (tmp_b && else_bb)
- {
- FOR_BB_INSNS (else_bb, tmp_insn)
- /* Don't check inside insn_b. We will have changed it to emit_b
- with a destination that doesn't conflict. */
- if (!(insn_b && tmp_insn == insn_b)
- && modified_in_p (orig_a, tmp_insn))
- {
- modified_in_b = true;
- break;
- }
-
- }
- if (modified_in_b)
- goto end_seq_and_fail;
-
- if (!noce_emit_bb (emit_b, else_bb, b_simple))
- goto end_seq_and_fail;
+ if (emit_a || modified_in_a)
+ {
+ modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
+ if (tmp_b && else_bb)
+ {
+ FOR_BB_INSNS (else_bb, tmp_insn)
+ /* Don't check inside insn_b. We will have changed it to emit_b
+ with a destination that doesn't conflict. */
+ if (!(insn_b && tmp_insn == insn_b)
+ && modified_in_p (orig_a, tmp_insn))
+ {
+ modified_in_b = true;
+ break;
+ }
+ }
+ if (modified_in_b)
+ goto end_seq_and_fail;
- if (!noce_emit_bb (emit_a, then_bb, a_simple))
- goto end_seq_and_fail;
- }
- else
- {
- if (!noce_emit_bb (emit_a, then_bb, a_simple))
- goto end_seq_and_fail;
+ if (!noce_emit_bb (emit_b, else_bb, b_simple))
+ goto end_seq_and_fail;
- if (!noce_emit_bb (emit_b, else_bb, b_simple))
- goto end_seq_and_fail;
+ if (!noce_emit_bb (emit_a, then_bb, a_simple))
+ goto end_seq_and_fail;
+ }
+ else
+ {
+ if (!noce_emit_bb (emit_a, then_bb, a_simple))
+ goto end_seq_and_fail;
- }
+ if (!noce_emit_bb (emit_b, else_bb, b_simple))
+ goto end_seq_and_fail;
+ }
target = noce_emit_cmove (if_info, x, code, XEXP (if_info->cond, 0),
XEXP (if_info->cond, 1), a, b);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68506.c b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
new file mode 100644
index 0000000..15984ed
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
@@ -0,0 +1,63 @@
+/* { dg-options "-fno-builtin-abort" } */
+
+int a, b, m, n, o, p, s, u, i;
+char c, q, y;
+short d;
+unsigned char e;
+static int f, h;
+static short g, r, v;
+unsigned t;
+
+extern void abort ();
+
+int
+fn1 (int p1)
+{
+ return a ? p1 : p1 + a;
+}
+
+unsigned char
+fn2 (unsigned char p1, int p2)
+{
+ return p2 >= 2 ? p1 : p1 >> p2;
+}
+
+static short
+fn3 ()
+{
+ int w, x = 0;
+ for (; p < 31; p++)
+ {
+ s = fn1 (c | ((1 && c) == c));
+ t = fn2 (s, x);
+ c = (unsigned) c > -(unsigned) ((o = (m = d = t) == p) <= 4UL) && n;
+ v = -c;
+ y = 1;
+ for (; y; y++)
+ e = v == 1;
+ d = 0;
+ for (; h != 2;)
+ {
+ for (;;)
+ {
+ if (!m)
+ abort ();
+ r = 7 - f;
+ x = e = i | r;
+ q = u * g;
+ w = b == q;
+ if (w)
+ break;
+ }
+ break;
+ }
+ }
+ return x;
+}
+
+int
+main ()
+{
+ fn3 ();
+ return 0;
+}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-27 9:45 ` Kyrill Tkachov
@ 2015-11-27 14:09 ` Richard Biener
2015-11-27 14:35 ` Kyrill Tkachov
0 siblings, 1 reply; 14+ messages in thread
From: Richard Biener @ 2015-11-27 14:09 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: Bernd Schmidt, GCC Patches
On Fri, Nov 27, 2015 at 10:44 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>
> On 26/11/15 16:54, Kyrill Tkachov wrote:
>>
>>
>> On 26/11/15 16:49, Bernd Schmidt wrote:
>>>
>>> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>>>>
>>>> that doesn't help, punt. */
>>>>
>>>> - modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>>>> if (tmp_b && then_bb)
>>>> {
>>>
>>> These bits I thought would be part of a followup patch (which would also
>>> guard against single_set problems), and as I mentioned I'd rather have a
>>> checking assert.
>>
>> Yes, you're right. I have the checking_assert statement in the followup
>> that I've been testing.
>> I'll move the deletion of these two statements there as well to minimise
>> the changes to this patch.
>>
>> I'll move these bits to that patch, re-build cc1 and commit.
>>
>
> Here it is.
> I'm committing this to trunk.
I think this causes
FAIL: gcc.c-torture/execute/20050124-1.c -O2 (internal compiler error)
FAIL: gcc.c-torture/execute/20050124-1.c -O2 (test for excess errors)
WARNING: gcc.c-torture/execute/20050124-1.c -O2 compilation failed to produce
executable
FAIL: gcc.c-torture/execute/20050124-1.c -O2 -flto -fno-use-linker-plugin -flt
o-partition=none (internal compiler error)
FAIL: gcc.c-torture/execute/20050124-1.c -O2 -flto -fno-use-linker-plugin -flt
o-partition=none (test for excess errors)
....
/space/rguenther/src/svn/trunk2/gcc/testsuite/gcc.c-torture/execute/20050124-1.c:19:1:
internal compiler error: in noce_try_cmove_arith, at ifcvt.c:2180^M
0x11f919d noce_try_cmove_arith^M
/space/rguenther/src/svn/trunk2/gcc/ifcvt.c:2180^M
0x11fb93f noce_process_if_block^M
/space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3525^M
0x11fdd0e noce_find_if_block^M
/space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3974^M
0x11fdd0e find_if_header^M
/space/rguenther/src/svn/trunk2/gcc/ifcvt.c:4179^M
0x11fdd0e if_convert^M
/space/rguenther/src/svn/trunk2/gcc/ifcvt.c:5326^M
0x11ff32d execute^M
on x86_64 with -m64 and -m32.
Richard.
> Thanks,
> Kyrill
>
> 2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>
> PR rtl-optimization/68506
> * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
> first if emit_a exists or then_bb modifies 'b'. Reindent if-else
> blocks.
>
> 2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>
> PR rtl-optimization/68506
> * gcc.c-torture/execute/pr68506.c: New test.
>
>> Thanks for your guidance,
>> Kyrill
>>
>>> So take these deletions out and leave them for the followup, and the
>>> patch is ok everywhere. No need for a full retest given that practically the
>>> same patch has been tested already, just make sure you can build cc1.
>>>
>>>
>>> Bernd
>>>
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-27 14:09 ` Richard Biener
@ 2015-11-27 14:35 ` Kyrill Tkachov
2015-11-27 14:38 ` Bernd Schmidt
0 siblings, 1 reply; 14+ messages in thread
From: Kyrill Tkachov @ 2015-11-27 14:35 UTC (permalink / raw)
To: Richard Biener; +Cc: Bernd Schmidt, GCC Patches
On 27/11/15 14:09, Richard Biener wrote:
> On Fri, Nov 27, 2015 at 10:44 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>> On 26/11/15 16:54, Kyrill Tkachov wrote:
>>>
>>> On 26/11/15 16:49, Bernd Schmidt wrote:
>>>> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>>>>> that doesn't help, punt. */
>>>>>
>>>>> - modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>>>>> if (tmp_b && then_bb)
>>>>> {
>>>> These bits I thought would be part of a followup patch (which would also
>>>> guard against single_set problems), and as I mentioned I'd rather have a
>>>> checking assert.
>>> Yes, you're right. I have the checking_assert statement in the followup
>>> that I've been testing.
>>> I'll move the deletion of these two statements there as well to minimise
>>> the changes to this patch.
>>>
>>> I'll move these bits to that patch, re-build cc1 and commit.
>>>
>> Here it is.
>> I'm committing this to trunk.
> I think this causes
>
> FAIL: gcc.c-torture/execute/20050124-1.c -O2 (internal compiler error)
> FAIL: gcc.c-torture/execute/20050124-1.c -O2 (test for excess errors)
> WARNING: gcc.c-torture/execute/20050124-1.c -O2 compilation failed to produce
> executable
> FAIL: gcc.c-torture/execute/20050124-1.c -O2 -flto -fno-use-linker-plugin -flt
> o-partition=none (internal compiler error)
> FAIL: gcc.c-torture/execute/20050124-1.c -O2 -flto -fno-use-linker-plugin -flt
> o-partition=none (test for excess errors)
> ....
Sorry for that.
That is caused not by this patch but rather by the followup
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html
The checking assert fails:
gcc_checking_assert (!emit_a || !modified_in_p (orig_b, emit_a));
emit_a is:
(parallel [
(set (reg:SI 93)
(plus:SI (reg/v:SI 88 [ i ])
(const_int 2 [0x2])))
(clobber (reg:CC 17 flags))
])
and and orig_b is:
(if_then_else:SI (eq (reg:CC 17 flags)
(const_int 0 [0]))
(reg/v:SI 87 [ <retval> ])
(reg/v:SI 88 [ i ]))
So I think our assumption that this case would never trigger by this point doesn't hold
due to the CC reg clobber.
So the code before that patch was probably correct.
I think we should revert https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html then.
Kyrill
> /space/rguenther/src/svn/trunk2/gcc/testsuite/gcc.c-torture/execute/20050124-1.c:19:1:
> internal compiler error: in noce_try_cmove_arith, at ifcvt.c:2180^M
> 0x11f919d noce_try_cmove_arith^M
> /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:2180^M
> 0x11fb93f noce_process_if_block^M
> /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3525^M
> 0x11fdd0e noce_find_if_block^M
> /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3974^M
> 0x11fdd0e find_if_header^M
> /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:4179^M
> 0x11fdd0e if_convert^M
> /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:5326^M
> 0x11ff32d execute^M
>
>
> on x86_64 with -m64 and -m32.
>
> Richard.
>
>> Thanks,
>> Kyrill
>>
>> 2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>
>> PR rtl-optimization/68506
>> * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
>> first if emit_a exists or then_bb modifies 'b'. Reindent if-else
>> blocks.
>>
>> 2015-11-26 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>
>> PR rtl-optimization/68506
>> * gcc.c-torture/execute/pr68506.c: New test.
>>
>>> Thanks for your guidance,
>>> Kyrill
>>>
>>>> So take these deletions out and leave them for the followup, and the
>>>> patch is ok everywhere. No need for a full retest given that practically the
>>>> same patch has been tested already, just make sure you can build cc1.
>>>>
>>>>
>>>> Bernd
>>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-27 14:35 ` Kyrill Tkachov
@ 2015-11-27 14:38 ` Bernd Schmidt
2015-11-27 14:41 ` Kyrill Tkachov
0 siblings, 1 reply; 14+ messages in thread
From: Bernd Schmidt @ 2015-11-27 14:38 UTC (permalink / raw)
To: Kyrill Tkachov, Richard Biener; +Cc: GCC Patches
On 11/27/2015 03:33 PM, Kyrill Tkachov wrote:
> Sorry for that.
> That is caused not by this patch but rather by the followup
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html
>
> The checking assert fails:
> gcc_checking_assert (!emit_a || !modified_in_p (orig_b, emit_a));
> emit_a is:
> (parallel [
> (set (reg:SI 93)
> (plus:SI (reg/v:SI 88 [ i ])
> (const_int 2 [0x2])))
> (clobber (reg:CC 17 flags))
> ])
>
> and and orig_b is:
> (if_then_else:SI (eq (reg:CC 17 flags)
> (const_int 0 [0]))
> (reg/v:SI 87 [ <retval> ])
> (reg/v:SI 88 [ i ]))
>
> So I think our assumption that this case would never trigger by this
> point doesn't hold
> due to the CC reg clobber.
> So the code before that patch was probably correct.
> I think we should revert
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html then.
Yes. Sorry. I thought orig_b would hold "normal" objects and not such an
if-then-else.
Bernd
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case
2015-11-27 14:38 ` Bernd Schmidt
@ 2015-11-27 14:41 ` Kyrill Tkachov
0 siblings, 0 replies; 14+ messages in thread
From: Kyrill Tkachov @ 2015-11-27 14:41 UTC (permalink / raw)
To: Bernd Schmidt, Richard Biener; +Cc: GCC Patches
On 27/11/15 14:35, Bernd Schmidt wrote:
> On 11/27/2015 03:33 PM, Kyrill Tkachov wrote:
>> Sorry for that.
>> That is caused not by this patch but rather by the followup
>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html
>>
>> The checking assert fails:
>> gcc_checking_assert (!emit_a || !modified_in_p (orig_b, emit_a));
>> emit_a is:
>> (parallel [
>> (set (reg:SI 93)
>> (plus:SI (reg/v:SI 88 [ i ])
>> (const_int 2 [0x2])))
>> (clobber (reg:CC 17 flags))
>> ])
>>
>> and and orig_b is:
>> (if_then_else:SI (eq (reg:CC 17 flags)
>> (const_int 0 [0]))
>> (reg/v:SI 87 [ <retval> ])
>> (reg/v:SI 88 [ i ]))
>>
>> So I think our assumption that this case would never trigger by this
>> point doesn't hold
>> due to the CC reg clobber.
>> So the code before that patch was probably correct.
>> I think we should revert
>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html then.
>
> Yes. Sorry. I thought orig_b would hold "normal" objects and not such an if-then-else.
>
Reverted with r231019.
Sorry for not catching it myself earlier.
Kyrill
>
> Bernd
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-11-27 14:41 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-26 11:15 [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case Kyrill Tkachov
2015-11-26 13:50 ` Bernd Schmidt
2015-11-26 14:00 ` Kyrill Tkachov
2015-11-26 14:27 ` Bernd Schmidt
2015-11-26 14:35 ` Kyrill Tkachov
2015-11-26 14:37 ` Bernd Schmidt
2015-11-26 16:49 ` Kyrill Tkachov
2015-11-26 16:51 ` Bernd Schmidt
2015-11-26 17:05 ` Kyrill Tkachov
2015-11-27 9:45 ` Kyrill Tkachov
2015-11-27 14:09 ` Richard Biener
2015-11-27 14:35 ` Kyrill Tkachov
2015-11-27 14:38 ` Bernd Schmidt
2015-11-27 14:41 ` 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).