Re: [PATCH 6/6] combine: allow combining two insns into two in some cases (PR52714)
On Wed, Dec 03, 2014 at 05:31:15PM +0800, Bin.Cheng wrote: > Does your series of patches affect PR62151? I am going to revisit > that one to see if I can answer Jeff's questions there. I don't think any of my recent patches affects it. r215789 does, however, so you'll need to revert that to show the problem again. To recap: We have five insns, call them A, B, C, D, E. A feeds B feeds C; C feeds both D and E. A and C set the same pseudo. D and E are simple moves. A+B+C would not combine because of a limitation that r215789 removed. Then combine tried combining A+B+C+D doing effectively the combination of A+B+C, and un-cse'ing the result to D. Then distribute_notes messes up with the REG_DEAD note of B (the result of A is dead); it takes it to mean the result of C is dead, but E still uses that. r215789 hides this in the cases where A+B+C in fact combines, which in practice is (nearly) all cases. Either we need to fix distribute_notes, or we should disallow A->B->C->D combos where the result of C is not dead at D, to fix this for real. I vote for the latter: 4-insn combine is expensive, and doesn't often improve anything, and it's even less likely to match anything in this case (which after all needs to keep the result of A+B+C around, even though that on its own did not combine!) Even if it matches, it in effect does an uncse which combine does not normally do; it would be better to do that in a separate pass (maybe piggyback it on fwprop, its sibling; or maybe do it separately. Something for the future, anyway). Segher
Re: [PATCH 6/6] combine: allow combining two insns into two in some cases (PR52714)
On Wed, Dec 3, 2014 at 1:54 PM, Segher Boessenkool wrote: > On Tue, Dec 02, 2014 at 07:11:04PM -0600, Segher Boessenkool wrote: >> Here is the testcase. I cannot actually test it on an m68k build, should >> really do something about that (I build lots of cross compilers but they >> cannot run the testsuite). Does it look okay, can you test it yourself? >> [I did of course test the generated asm is indeed okay; just cannot test >> the dejagnu stuff]. > > Note to self: "make check-gcc RUNTESTFLAGS=m68k.exp=pr52714.c" and don't > forget that _yet again_. > > Testcase fails without the patch, works with, installing. > > Thanks for all the reviewing, > > > Segher Hi Segher, Does your series of patches affect PR62151? I am going to revisit that one to see if I can answer Jeff's questions there. Thanks, bin
Re: [PATCH 6/6] combine: allow combining two insns into two in some cases (PR52714)
On 12/02/14 18:11, Segher Boessenkool wrote: On Mon, Dec 01, 2014 at 10:39:58AM -0700, Jeff Law wrote: Also OK with a testcase. I'd recommend just using the one from 52714, make it m68k specific. Not sure if it's best to scan the assembly or .combine dump -- your call. Here is the testcase. I cannot actually test it on an m68k build, should really do something about that (I build lots of cross compilers but they cannot run the testsuite). Does it look okay, can you test it yourself? [I did of course test the generated asm is indeed okay; just cannot test the dejagnu stuff]. Segher 2014-12-02 Segher Boessenkool gcc/testsuite/ PR rtl-optimization/52714 * gcc.target/m68k/pr52714.c: New testcase. THanks. Based on a later message, you were able to do the testing. At some point during the gcc-5 release process, I'll bootstrap m68k via Aranym. Jeff
Re: [PATCH 6/6] combine: allow combining two insns into two in some cases (PR52714)
On Tue, Dec 02, 2014 at 07:11:04PM -0600, Segher Boessenkool wrote: > Here is the testcase. I cannot actually test it on an m68k build, should > really do something about that (I build lots of cross compilers but they > cannot run the testsuite). Does it look okay, can you test it yourself? > [I did of course test the generated asm is indeed okay; just cannot test > the dejagnu stuff]. Note to self: "make check-gcc RUNTESTFLAGS=m68k.exp=pr52714.c" and don't forget that _yet again_. Testcase fails without the patch, works with, installing. Thanks for all the reviewing, Segher
Re: [PATCH 6/6] combine: allow combining two insns into two in some cases (PR52714)
On Mon, Dec 01, 2014 at 10:39:58AM -0700, Jeff Law wrote:
> Also OK with a testcase. I'd recommend just using the one from 52714,
> make it m68k specific. Not sure if it's best to scan the assembly or
> .combine dump -- your call.
Here is the testcase. I cannot actually test it on an m68k build, should
really do something about that (I build lots of cross compilers but they
cannot run the testsuite). Does it look okay, can you test it yourself?
[I did of course test the generated asm is indeed okay; just cannot test
the dejagnu stuff].
Segher
2014-12-02 Segher Boessenkool
gcc/testsuite/
PR rtl-optimization/52714
* gcc.target/m68k/pr52714.c: New testcase.
---
gcc/testsuite/gcc.target/m68k/pr52714.c | 33 +
1 file changed, 33 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/m68k/pr52714.c
diff --git a/gcc/testsuite/gcc.target/m68k/pr52714.c
b/gcc/testsuite/gcc.target/m68k/pr52714.c
new file mode 100644
index 000..0a52a1d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/m68k/pr52714.c
@@ -0,0 +1,33 @@
+/* PR rtl-optimization/52714
+
+ Check that combine manages to remove the "stack == 0" test.
+ Without ICEing. */
+
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+int __re_compile_fastmap(unsigned char *p)
+{
+unsigned char **stack;
+unsigned size;
+unsigned avail;
+
+stack = __builtin_alloca(5 * sizeof(unsigned char*));
+if (stack == 0)
+ return -2;
+size = 5;
+avail = 0;
+
+for (;;) {
+ switch (*p++) {
+ case 0:
+ if (avail == size)
+ return -2;
+ stack[avail++] = p;
+ }
+}
+
+return 0;
+}
+
+/* { dg-final { scan-assembler-not {\mtst\.l %sp\M} } } */
--
1.8.1.4
Re: [PATCH 6/6] combine: allow combining two insns into two in some cases (PR52714)
On 11/27/14 18:44, Segher Boessenkool wrote: If one of the resulting insns is a noop set it is safe to allow the combination, for it will never lead to a loop: any following combination using that noop will delete it. In fact, in most cases combine deletes the noop immediately; for cc0 targets it does not in some cases, but it deletes another insn in that case (a following jump insn). This fixes PR52714. 2014-11-27 Segher Boessenkool gcc/ PR rtl-optimization/52714 * combine.c (try_combine): Allow combining two insns into two new insns if at least one of those is a noop. Also OK with a testcase. I'd recommend just using the one from 52714, make it m68k specific. Not sure if it's best to scan the assembly or .combine dump -- your call. jeff
[PATCH 6/6] combine: allow combining two insns into two in some cases (PR52714)
If one of the resulting insns is a noop set it is safe to allow the combination, for it will never lead to a loop: any following combination using that noop will delete it. In fact, in most cases combine deletes the noop immediately; for cc0 targets it does not in some cases, but it deletes another insn in that case (a following jump insn). This fixes PR52714. 2014-11-27 Segher Boessenkool gcc/ PR rtl-optimization/52714 * combine.c (try_combine): Allow combining two insns into two new insns if at least one of those is a noop. --- gcc/combine.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/gcc/combine.c b/gcc/combine.c index 91ddff4..66007d8 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -3812,15 +3812,20 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, /* Similarly, check for a case where we have a PARALLEL of two independent SETs but we started with three insns. In this case, we can do the sets as two separate insns. This case occurs when some SET allows two - other insns to combine, but the destination of that SET is still live. */ + other insns to combine, but the destination of that SET is still live. - else if (i1 && insn_code_number < 0 && asm_noperands (newpat) < 0 + Also do this if we started with two insns and (at least) one of the + resulting sets is a noop; this noop will be deleted later. */ + + else if (insn_code_number < 0 && asm_noperands (newpat) < 0 && GET_CODE (newpat) == PARALLEL && XVECLEN (newpat, 0) == 2 && GET_CODE (XVECEXP (newpat, 0, 0)) == SET + && GET_CODE (XVECEXP (newpat, 0, 1)) == SET + && (i1 || set_noop_p (XVECEXP (newpat, 0, 0)) + || set_noop_p (XVECEXP (newpat, 0, 1))) && GET_CODE (SET_DEST (XVECEXP (newpat, 0, 0))) != ZERO_EXTRACT && GET_CODE (SET_DEST (XVECEXP (newpat, 0, 0))) != STRICT_LOW_PART - && GET_CODE (XVECEXP (newpat, 0, 1)) == SET && GET_CODE (SET_DEST (XVECEXP (newpat, 0, 1))) != ZERO_EXTRACT && GET_CODE (SET_DEST (XVECEXP (newpat, 0, 1))) != STRICT_LOW_PART && ! reg_referenced_p (SET_DEST (XVECEXP (newpat, 0, 1)), -- 1.8.1.4
