Re: [PATCH]Partially fix PR61529, bound basic block frequency
Hi Renlin, Are the incoming edge counts or probabilities insane in this case? I guess the patch is ok if we need to do this to handle those incoming insanitiles. But I can't approve patches myself. However, this is a fix to code (r215739) committed after the ICE in the original bug report and in comment 2 were reported, so I wonder if it is just hiding the original problem. Originally this was reported to be due to r210538 - ccing Dehao who was the author of that patch. Dehao, did you get a chance to look at this bug and see why your change triggered it? It is possible that Dehao's patch simply amplified an even further upstream profile insanity, but it would be good to confirm. Thanks! Teresa On Wed, Oct 29, 2014 at 2:26 AM, Renlin Li wrote: > Hi all, > > This is a simple patch to fix ICE in comment 2 of PR61529: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61529 > > Bound checking code is added to make sure the frequency is within legal > range. > > As far as I have observed, r215830 patch fixes the glibc building ICE. And > this patch should fix the ICE while building the sample code in comment 2 > using aarch64-none-elf toolchain. Until now, all the ICEs reported in this > bug ticket should be fixed. > > x86_64-unknown-linux-gnu bootstrap and regression test have been done, no > new issue. > aarch64-none-elf toolchain has been test on the model. No new regression. > > Is this Okay for trunk? > > gcc/ChangeLog: > > 2014-10-29 Renlin Li > PR middle-end/61529 > * tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq. -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH]Partially fix PR61529, bound basic block frequency
On 29/10/14 12:42, Teresa Johnson wrote: Hi Renlin, Are the incoming edge counts or probabilities insane in this case? I guess the patch is ok if we need to do this to handle those incoming insanitiles. But I can't approve patches myself. Not really, it's just a little bigger than the limit. For this particular test case, ABC is a threaded path. B is the fallthrough basic block of A, D is a basic block split from B (used to be a self loop). A, B and D have roughly the same frequency ( 8281, 9100, 8281). When calculating the path_in_freq, frequencies from AB and DB edges are accumulated, and the final result is large than BB_FREQ_MAX. A 100% | | 9% -->B-->C | | |100%| 91% | | D There are 2 suspicious points: 1, The BD edge is not correctly guessed at the profile stage. However, anyway it's heuristic, so I don't think, it's here the problem starts. 2, The BD edge is not eliminated before jump threading. But the jump threading pass will analysis the condition jump statement in B block (In this particular case, the BD probability should be zero), and makes the decision to thread it. Later in the dom pass, the BD edge is indeed removed. However, this is a fix to code (r215739) committed after the ICE in the original bug report and in comment 2 were reported, so I wonder if it is just hiding the original problem. Originally this was reported to be due to r210538 - ccing Dehao who was the author of that patch. Dehao, did you get a chance to look at this bug and see why your change triggered it? It is possible that Dehao's patch simply amplified an even further upstream profile insanity, but it would be good to confirm. Thanks! Teresa On Wed, Oct 29, 2014 at 2:26 AM, Renlin Li wrote: Hi all, This is a simple patch to fix ICE in comment 2 of PR61529: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61529 Bound checking code is added to make sure the frequency is within legal range. As far as I have observed, r215830 patch fixes the glibc building ICE. And this patch should fix the ICE while building the sample code in comment 2 using aarch64-none-elf toolchain. Until now, all the ICEs reported in this bug ticket should be fixed. x86_64-unknown-linux-gnu bootstrap and regression test have been done, no new issue. aarch64-none-elf toolchain has been test on the model. No new regression. Is this Okay for trunk? gcc/ChangeLog: 2014-10-29 Renlin Li PR middle-end/61529 * tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq.
Re: [PATCH]Partially fix PR61529, bound basic block frequency
On 11/03/14 08:29, Renlin Li wrote: On 29/10/14 12:42, Teresa Johnson wrote: Hi Renlin, Are the incoming edge counts or probabilities insane in this case? I guess the patch is ok if we need to do this to handle those incoming insanitiles. But I can't approve patches myself. Not really, it's just a little bigger than the limit. For this particular test case, ABC is a threaded path. B is the fallthrough basic block of A, D is a basic block split from B (used to be a self loop). A, B and D have roughly the same frequency ( 8281, 9100, 8281). When calculating the path_in_freq, frequencies from AB and DB edges are accumulated, and the final result is large than BB_FREQ_MAX. A 100% | | 9% -->B-->C | | |100%| 91% | | D There are 2 suspicious points: 1, The BD edge is not correctly guessed at the profile stage. However, anyway it's heuristic, so I don't think, it's here the problem starts. 2, The BD edge is not eliminated before jump threading. But the jump threading pass will analysis the condition jump statement in B block (In this particular case, the BD probability should be zero), and makes the decision to thread it. Later in the dom pass, the BD edge is indeed removed. Can you add a testcase please? With a testcase, this patch is OK for the trunk. jeff
Re: [PATCH]Partially fix PR61529, bound basic block frequency
Hi Jeff,
Test case has been added. With the patch, both x86_64-unknown-linux-gnu
and aarch64-none-elf compile the test case successfully.
Okay to commit?
On 04/11/14 21:59, Jeff Law wrote:
On 11/03/14 08:29, Renlin Li wrote:
On 29/10/14 12:42, Teresa Johnson wrote:
Hi Renlin,
Are the incoming edge counts or probabilities insane in this case? I
guess the patch is ok if we need to do this to handle those incoming
insanitiles. But I can't approve patches myself.
Not really, it's just a little bigger than the limit.
For this particular test case, ABC is a threaded path.
B is the fallthrough basic block of A, D is a basic block split from B
(used to be a self loop). A, B and D have roughly the same frequency (
8281, 9100, 8281).
When calculating the path_in_freq, frequencies from AB and DB edges are
accumulated, and the final result is large than BB_FREQ_MAX.
A
100% |
| 9%
-->B-->C
| |
|100%| 91%
| |
D
There are 2 suspicious points:
1, The BD edge is not correctly guessed at the profile stage. However,
anyway it's heuristic, so I don't think, it's here the problem starts.
2, The BD edge is not eliminated before jump threading. But the jump
threading pass will analysis the condition jump statement in B block (In
this particular case, the BD probability should be zero), and makes the
decision to thread it.
Later in the dom pass, the BD edge is indeed removed.
Can you add a testcase please? With a testcase, this patch is OK for
the trunk.
jeff
x86_64-unknown-linux-gnu bootstrap and regression test have been done,
no new issue.
aarch64-none-elf toolchain has been test on the model. No new regression.
gcc/ChangeLog:
2014-11-06 Renlin Li
PR middle-end/61529
* tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq.
gcc/testsuite/ChangeLog:
2014-11-06 Renlin Li
PR middle-end/61529
* gcc.dg/pr61529.c: New.commit fead29d30b2985a1ba338759054f99d71d81f3c0
Author: Renlin Li
Date: Tue Oct 28 16:30:42 2014 +
fix pr61529
diff --git a/gcc/testsuite/gcc.dg/pr61529.c b/gcc/testsuite/gcc.dg/pr61529.c
new file mode 100644
index 000..9ae2e06
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr61529.c
@@ -0,0 +1,26 @@
+/* PR middle-end/61529 */
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+unsigned int a, b, c;
+
+int
+main ()
+{
+ unsigned int d;
+ int e[5];
+
+ for (; b < 1; b++)
+d = 0;
+ for (; d < 1; d++)
+a = 0;
+ for (; a < 1; a++)
+;
+
+ for (c = 0; c < 5; c++)
+e[c] = 1;
+ if (e[0])
+c = 0;
+
+ return 0;
+}
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index d2cf4de..e3077a1 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -730,6 +730,10 @@ compute_path_counts (struct redirection_data *rd,
nonpath_count += ein->count;
}
}
+
+ if (path_in_freq > BB_FREQ_MAX)
+path_in_freq = BB_FREQ_MAX;
+
BITMAP_FREE (in_edge_srcs);
/* Now compute the fraction of the total count coming into the first
Re: [PATCH]Partially fix PR61529, bound basic block frequency
On Thu, Nov 6, 2014 at 7:09 AM, Renlin Li wrote: > > Hi Jeff, > > Test case has been added. With the patch, both x86_64-unknown-linux-gnu and > aarch64-none-elf compile the test case successfully. > > Okay to commit? > > > On 04/11/14 21:59, Jeff Law wrote: >> >> On 11/03/14 08:29, Renlin Li wrote: >>> >>> On 29/10/14 12:42, Teresa Johnson wrote: Hi Renlin, Are the incoming edge counts or probabilities insane in this case? I guess the patch is ok if we need to do this to handle those incoming insanitiles. But I can't approve patches myself. >>> >>> Not really, it's just a little bigger than the limit. >>> >>> For this particular test case, ABC is a threaded path. >>> B is the fallthrough basic block of A, D is a basic block split from B >>> (used to be a self loop). A, B and D have roughly the same frequency ( >>> 8281, 9100, 8281). >>> When calculating the path_in_freq, frequencies from AB and DB edges are >>> accumulated, and the final result is large than BB_FREQ_MAX. >>> >>> >>> A >>> 100% | >>> | 9% >>> -->B-->C >>> | | >>> |100%| 91% >>> | | >>> D The frequencies look insane given these probabilities. If most of the execution stays in the loop then B should have a much higher frequency than A. >>> >>> >>> >>> There are 2 suspicious points: >>> 1, The BD edge is not correctly guessed at the profile stage. However, >>> anyway it's heuristic, so I don't think, it's here the problem starts. >>> 2, The BD edge is not eliminated before jump threading. But the jump >>> threading pass will analysis the condition jump statement in B block (In >>> this particular case, the BD probability should be zero), and makes the >>> decision to thread it. >>> >>> Later in the dom pass, the BD edge is indeed removed. >> >> Can you add a testcase please? With a testcase, this patch is OK for >> the trunk. >> >> jeff >> > > x86_64-unknown-linux-gnu bootstrap and regression test have been done, no > new issue. > aarch64-none-elf toolchain has been test on the model. No new regression. > > gcc/ChangeLog: > > 2014-11-06 Renlin Li > PR middle-end/61529 > * tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq. Please add a comment that this is needed due to insane incoming frequencies. > > gcc/testsuite/ChangeLog: > > 2014-11-06 Renlin Li > PR middle-end/61529 > * gcc.dg/pr61529.c: New. The 'b' variable is uninitialized. Also, 'd' and 'a' may end up uninitialized depending on the initial value of 'b'. Please initialize these. Thanks, Teresa -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH]Partially fix PR61529, bound basic block frequency
Hi Teresa,
Thank you for the suggestion, updated!
Please add a comment that this is needed due to insane incoming frequencies.
The 'b' variable is uninitialized. Also, 'd' and 'a' may end up
uninitialized depending on the initial value of 'b'. Please initialize
these.
Test case has been added. With the patch, both x86_64-unknown-linux-gnu
and aarch64-none-elf compile the test case successfully.
x86_64-unknown-linux-gnu bootstrap and regression test have been done,
no new issue.
aarch64-none-elf toolchain has been test on the model. No new regression.
gcc/ChangeLog:
2014-11-06 Renlin Li
PR middle-end/61529
* tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq.
This is needed due to insane incoming frequencies.
gcc/testsuite/ChangeLog:
2014-11-06 Renlin Li
PR middle-end/61529
* gcc.dg/pr61529.c: New.commit b38cf02619f03be9d200849734f4454502d1e5ac
Author: Renlin Li
Date: Tue Oct 28 16:30:42 2014 +
fix pr61529
diff --git a/gcc/testsuite/gcc.dg/pr61529.c b/gcc/testsuite/gcc.dg/pr61529.c
new file mode 100644
index 000..392239e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr61529.c
@@ -0,0 +1,27 @@
+/* PR middle-end/61529 */
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+unsigned int a = 0, b = 0;
+unsigned int c;
+
+int
+main ()
+{
+ unsigned int d = 0;
+ int e[5];
+
+ for (; b < 1; b++)
+d = 0;
+ for (; d < 1; d++)
+a = 0;
+ for (; a < 1; a++)
+;
+
+ for (c = 0; c < 5; c++)
+e[c] = 1;
+ if (e[0])
+c = 0;
+
+ return 0;
+}
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index d2cf4de..e3077a1 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -730,6 +730,10 @@ compute_path_counts (struct redirection_data *rd,
nonpath_count += ein->count;
}
}
+
+ if (path_in_freq > BB_FREQ_MAX)
+path_in_freq = BB_FREQ_MAX;
+
BITMAP_FREE (in_edge_srcs);
/* Now compute the fraction of the total count coming into the first
Re: [PATCH]Partially fix PR61529, bound basic block frequency
Thanks for fixing the test case. Can you also add the comment I suggested to the source change? > Please add a comment that this is needed due to insane incoming frequencies. Thanks, Teresa On Thu, Nov 6, 2014 at 9:53 AM, Renlin Li wrote: > Hi Teresa, > > Thank you for the suggestion, updated! > >> Please add a comment that this is needed due to insane incoming >> frequencies. >> >> The 'b' variable is uninitialized. Also, 'd' and 'a' may end up >> uninitialized depending on the initial value of 'b'. Please initialize >> these. >> > > Test case has been added. With the patch, both x86_64-unknown-linux-gnu and > aarch64-none-elf compile the test case successfully. > > x86_64-unknown-linux-gnu bootstrap and regression test have been done, no > new issue. > aarch64-none-elf toolchain has been test on the model. No new regression. > > gcc/ChangeLog: > > 2014-11-06 Renlin Li > PR middle-end/61529 > * tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq. > This is needed due to insane incoming frequencies. > > gcc/testsuite/ChangeLog: > > 2014-11-06 Renlin Li > PR middle-end/61529 > * gcc.dg/pr61529.c: New. -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
Re: [PATCH]Partially fix PR61529, bound basic block frequency
On 06/11/14 17:59, Teresa Johnson wrote:
Thanks for fixing the test case. Can you also add the comment I
suggested to the source change?
Please add a comment that this is needed due to insane incoming frequencies.
Sorry, I mistakenly add it to the ChangeLog. Should be correct now.
x86_64-unknown-linux-gnu bootstrap and regression test have been done,
no new issue.
aarch64-none-elf toolchain has been test on the model. No new regression.
gcc/ChangeLog:
2014-11-06 Renlin Li
PR middle-end/61529
* tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq.
gcc/testsuite/ChangeLog:
2014-11-06 Renlin Li
PR middle-end/61529
* gcc.dg/pr61529.c: New.commit 35614b9b49d2478ad30225e4a3e864cd5df2e67f
Author: Renlin Li
Date: Tue Oct 28 16:30:42 2014 +
fix pr61529
diff --git a/gcc/testsuite/gcc.dg/pr61529.c b/gcc/testsuite/gcc.dg/pr61529.c
new file mode 100644
index 000..392239e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr61529.c
@@ -0,0 +1,27 @@
+/* PR middle-end/61529 */
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+unsigned int a = 0, b = 0;
+unsigned int c;
+
+int
+main ()
+{
+ unsigned int d = 0;
+ int e[5];
+
+ for (; b < 1; b++)
+d = 0;
+ for (; d < 1; d++)
+a = 0;
+ for (; a < 1; a++)
+;
+
+ for (c = 0; c < 5; c++)
+e[c] = 1;
+ if (e[0])
+c = 0;
+
+ return 0;
+}
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index d2cf4de..3e20916 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -730,6 +730,11 @@ compute_path_counts (struct redirection_data *rd,
nonpath_count += ein->count;
}
}
+
+ /* This is needed due to insane incoming frequencies. */
+ if (path_in_freq > BB_FREQ_MAX)
+path_in_freq = BB_FREQ_MAX;
+
BITMAP_FREE (in_edge_srcs);
/* Now compute the fraction of the total count coming into the first
