On Thu, Oct 2, 2025 at 6:22 PM Jan Hubicka <[email protected]> wrote: > > > Hi Jan, > > > > I didn't find > > > > https://gcc.gnu.org/cgit/gcc/commit/?id=8498ef3d075801 > > > > in > > > > https://gcc.gnu.org/pipermail/gcc-patches/2025-October/date.html > > > > In any case, your commit caused: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122122 > > Hello, > I apologize for this. The patch in question was the following. I am > not sure why it did not read mailing list. > > Improve profile update in merge_blocks > > When merging blocks we currently alway use count of the first basic block. > In some cases we merge block containing call to cold noreturn function (thus > having count 0 (reliable)) with earlier block with weaker form of profile. > In this case we can still preserve reliable count of 0. > > The patch also makes block merging to pick higher of the counts if quality > is the same. This should reduce chances of losing track of hot code in broken > profiles. > > Bootstrapped/regtested x86_64-linux, comitted. > > gcc/ChangeLog: > > * cfghooks.cc (merge_blocks): Choose more reliable or higher BB > count. > > diff --git a/gcc/cfghooks.cc b/gcc/cfghooks.cc > index 5f7fc272374..8b3346898aa 100644 > --- a/gcc/cfghooks.cc > +++ b/gcc/cfghooks.cc > @@ -817,6 +817,15 @@ merge_blocks (basic_block a, basic_block b) > if (!cfg_hooks->merge_blocks) > internal_error ("%s does not support merge_blocks", cfg_hooks->name); > > + /* Pick the more reliable count. If both qualities agrees, pick the larger > + one since turning mistakely hot code to cold is more harmful. */ > + if (a->count.initialized_p ()) > + a->count = b->count; > + else if (a->count.quality () < b->count.quality ()) > + a->count = b->count; > + else if (a->count.quality () == b->count.quality ()) > + a->count = a->count.max (b->count); > + > cfg_hooks->merge_blocks (a, b); > > if (current_loops != NULL) > > > Now the bug is reversed first conditional about unitialized. If > a->count is unitialized while b->count is, we are better to use it. > While I had rest of the patch in my tree for a while, I added this test > late and missing !. Thanks for spotting it. > I see that your patch also tests for block b being non-empty. > I am not sure this is a good idea. Even empty block can hold meaningful > profile. We can experiment with that separately.
if (!a->count.initialized_p ()) alone doesn't fix all regressions in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122122 Ignoring empty blocks fixes all of them without introducing any regressions. > I am testing the obvious fix and will commit it > > Fix handling of uninitialized counts in merge_blocks > > gcc/ChangeLog: > > * cfghooks.cc (merge_blocks): Fix typo in the previous change. > > Co-authored-by: H.J. Lu <[email protected]> > > > diff --git a/gcc/cfghooks.cc b/gcc/cfghooks.cc > index 8b3346898aa..25bc5d4b273 100644 > --- a/gcc/cfghooks.cc > +++ b/gcc/cfghooks.cc > @@ -819,7 +819,7 @@ merge_blocks (basic_block a, basic_block b) > > /* Pick the more reliable count. If both qualities agrees, pick the larger > one since turning mistakely hot code to cold is more harmful. */ > - if (a->count.initialized_p ()) > + if (!a->count.initialized_p ()) > a->count = b->count; > else if (a->count.quality () < b->count.quality ()) > a->count = b->count; -- H.J.
