| Message ID | 5450B2C0.6040807@arm.com |
|---|---|
| State | New |
| Headers | show |
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 <renlin.li@arm.com> 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 <Renlin.Li@arm.com> > PR middle-end/61529 > * tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq.
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<renlin.li@arm.com> 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<Renlin.Li@arm.com> >> PR middle-end/61529 >> * tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq.
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
commit c44195cb52ec8ac6386b2b7afe467b680422fb2e
Author: Renlin Li <renlin.li@arm.com>
Date: Tue Oct 28 16:30:42 2014 +0000
fix pr61529
Change-Id: Ie5e58510f21a4d7a609306006270c3168ab48d06
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
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 <Renlin.Li@arm.com> PR middle-end/61529 * tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq.