| Message ID | 55EDBB73.5080900@arm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 7, 2015 at 9:29 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > Hi all, > > This patch fixes the PRs in the ChangeLog that have been reported against my > if-conversion patch. > The problem occurs when the 'then' block is complex but the else block is > empty. > In this case the calling code in noce_process_if_block takes the 'else' move > (x := b) from > the test block. However, we have not checked whether the test block is valid > for complex-block > if-conversion with bb_valid_for_noce_process_p. Also, that's a case I wasn't > particularly targeting > when writing the initial patch. > > This patch bails out of noce_try_cmove_arith when one of the blocks is > complex and the other is empty. > I've checked that if-conversion still happens in the cases of interest from > the original patch. > > I've added the testcase from PR 67465 since that one uses __builtin_abort > and triggers the problem nicely. > The others show the miscompilation using printf seems to go away if I > replace it with an abort. > I have confirmed manually that the miscompilation goes away on those > testcases. > > PR rtl-optimization/67481 is a testsuite regression on sparc-solaris that > Rainer reported. I haven't tested > that this patch fixes that, but I suspect that the root cause is the same. > Rainer, could you please > check that this fixes the regression for you? > > Bootstrapped and tested on aarch64 and x86_64. > > Ok for trunk if sparc testing comes ok? > > Thanks, > Kyrill > > 2015-09-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/67456 > PR rtl-optimization/67464 > PR rtl-optimization/67465 > PR rtl-optimization/67481 > * ifcvt.c (noce_try_cmove_arith): Bail out if one of the blocks > is complex and the other is empty. > > 2015-09-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * gcc.dg/pr67465.c: New test. Does it fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67462
On 07/09/15 20:14, H.J. Lu wrote: > On Mon, Sep 7, 2015 at 9:29 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> Hi all, >> >> This patch fixes the PRs in the ChangeLog that have been reported against my >> if-conversion patch. >> The problem occurs when the 'then' block is complex but the else block is >> empty. >> In this case the calling code in noce_process_if_block takes the 'else' move >> (x := b) from >> the test block. However, we have not checked whether the test block is valid >> for complex-block >> if-conversion with bb_valid_for_noce_process_p. Also, that's a case I wasn't >> particularly targeting >> when writing the initial patch. >> >> This patch bails out of noce_try_cmove_arith when one of the blocks is >> complex and the other is empty. >> I've checked that if-conversion still happens in the cases of interest from >> the original patch. >> >> I've added the testcase from PR 67465 since that one uses __builtin_abort >> and triggers the problem nicely. >> The others show the miscompilation using printf seems to go away if I >> replace it with an abort. >> I have confirmed manually that the miscompilation goes away on those >> testcases. >> >> PR rtl-optimization/67481 is a testsuite regression on sparc-solaris that >> Rainer reported. I haven't tested >> that this patch fixes that, but I suspect that the root cause is the same. >> Rainer, could you please >> check that this fixes the regression for you? >> >> Bootstrapped and tested on aarch64 and x86_64. >> >> Ok for trunk if sparc testing comes ok? >> >> Thanks, >> Kyrill >> >> 2015-09-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR rtl-optimization/67456 >> PR rtl-optimization/67464 >> PR rtl-optimization/67465 >> PR rtl-optimization/67481 >> * ifcvt.c (noce_try_cmove_arith): Bail out if one of the blocks >> is complex and the other is empty. >> >> 2015-09-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> * gcc.dg/pr67465.c: New test. > Does it fix > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67462 No, PR 67462 is a testism. I've added a comment to the issue with my thoughts. Kyrill > >
Hi Kyrill, > PR rtl-optimization/67481 is a testsuite regression on sparc-solaris that > Rainer reported. I haven't tested > that this patch fixes that, but I suspect that the root cause is the > same. Rainer, could you please > check that this fixes the regression for you? I've now checked that with your patch the regression went away indeed, using a limited non-bootstrap build on sparc-sun-solaris2.10. Next I'll run a full bootstrap to check there are no other issues. Thanks. Rainer
On 08/09/15 10:26, Rainer Orth wrote: > Hi Kyrill, > >> PR rtl-optimization/67481 is a testsuite regression on sparc-solaris that >> Rainer reported. I haven't tested >> that this patch fixes that, but I suspect that the root cause is the >> same. Rainer, could you please >> check that this fixes the regression for you? > I've now checked that with your patch the regression went away indeed, > using a limited non-bootstrap build on sparc-sun-solaris2.10. Next I'll > run a full bootstrap to check there are no other issues. After some more benchmarking I've noticed that this patch is overly restrictive in some cases. I have a prototype patch that fixes the regressions and does not restrict if-conversion too much. I need to do some more testing, and hope to post it in due time. Sorry for the noise. Kyrill > > Thanks. > Rainer >
Kyrill Tkachov <kyrylo.tkachov@arm.com> writes: > On 08/09/15 10:26, Rainer Orth wrote: >> Hi Kyrill, >> >>> PR rtl-optimization/67481 is a testsuite regression on sparc-solaris that >>> Rainer reported. I haven't tested >>> that this patch fixes that, but I suspect that the root cause is the >>> same. Rainer, could you please >>> check that this fixes the regression for you? >> I've now checked that with your patch the regression went away indeed, >> using a limited non-bootstrap build on sparc-sun-solaris2.10. Next I'll >> run a full bootstrap to check there are no other issues. > > After some more benchmarking I've noticed that this patch is overly restrictive > in some cases. I have a prototype patch that fixes the regressions and does not > restrict if-conversion too much. > > I need to do some more testing, and hope to post it in due time. FWIW, the bootstrap with your original patch has now completed successfully, the only change being that the testsuite failures are gone. Rainer
commit 2305f7deed793315f04221f718880676cd62474d
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Mon Sep 7 14:58:01 2015 +0100
[RTL-ifcvt] PR rtl-optimization/67465: Do not ifcvt complex blocks if the else block is empty
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index d2f5b66..5716dcc 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2079,7 +2079,14 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
}
}
- if (!a_simple && then_bb && !b_simple && else_bb
+ /* When one of the blocks is really empty and the other is a complex block
+ don't do anything. The value 'b' may have come from the test block that
+ we did not check for if-conversion validity in noce_process_if_block. */
+ if ((!a_simple && !else_bb)
+ || (!b_simple && !then_bb))
+ return FALSE;
+
+ if (then_bb && else_bb
&& (!bbs_ok_for_cmove_arith (then_bb, else_bb)
|| !bbs_ok_for_cmove_arith (else_bb, then_bb)))
return FALSE;
diff --git a/gcc/testsuite/gcc.dg/pr67465.c b/gcc/testsuite/gcc.dg/pr67465.c
new file mode 100644
index 0000000..321fd38
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr67465.c
@@ -0,0 +1,53 @@
+/* { dg-do run } */
+/* { dg-options "-O3 -std=gnu99" } */
+
+int a, b, c, d, e, h;
+
+int
+fn1 (int p1)
+{
+ {
+ int g[2];
+ for (int i = 0; i < 1; i++)
+ g[i] = 0;
+ if (g[0] < c)
+ {
+ a = (unsigned) (1 ^ p1) % 2;
+ return 0;
+ }
+ }
+ return 0;
+}
+
+void
+fn2 ()
+{
+ for (h = 0; h < 1; h++)
+ {
+ for (int j = 0; j < 2; j++)
+ {
+ for (b = 1; b; b = 0)
+ a = 1;
+ for (; b < 1; b++)
+ ;
+ if (e)
+ continue;
+ a = 2;
+ }
+ fn1 (h);
+ short k = -16;
+ d = k > a;
+ }
+}
+
+int
+main ()
+{
+ fn2 ();
+
+ if (a != 2)
+ __builtin_abort ();
+
+ return 0;
+}
+
Hi all, This patch fixes the PRs in the ChangeLog that have been reported against my if-conversion patch. The problem occurs when the 'then' block is complex but the else block is empty. In this case the calling code in noce_process_if_block takes the 'else' move (x := b) from the test block. However, we have not checked whether the test block is valid for complex-block if-conversion with bb_valid_for_noce_process_p. Also, that's a case I wasn't particularly targeting when writing the initial patch. This patch bails out of noce_try_cmove_arith when one of the blocks is complex and the other is empty. I've checked that if-conversion still happens in the cases of interest from the original patch. I've added the testcase from PR 67465 since that one uses __builtin_abort and triggers the problem nicely. The others show the miscompilation using printf seems to go away if I replace it with an abort. I have confirmed manually that the miscompilation goes away on those testcases. PR rtl-optimization/67481 is a testsuite regression on sparc-solaris that Rainer reported. I haven't tested that this patch fixes that, but I suspect that the root cause is the same. Rainer, could you please check that this fixes the regression for you? Bootstrapped and tested on aarch64 and x86_64. Ok for trunk if sparc testing comes ok? Thanks, Kyrill 2015-09-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR rtl-optimization/67456 PR rtl-optimization/67464 PR rtl-optimization/67465 PR rtl-optimization/67481 * ifcvt.c (noce_try_cmove_arith): Bail out if one of the blocks is complex and the other is empty. 2015-09-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * gcc.dg/pr67465.c: New test.