On Tue, Dec 16, 2025 at 3:37 PM Andrew Pinski
<[email protected]> wrote:
>
> On Tue, Dec 16, 2025 at 4:14 AM Richard Biener
> <[email protected]> wrote:
> >
> > On Tue, Dec 16, 2025 at 9:48 AM Andrew Pinski
> > <[email protected]> wrote:
> > >
> > > On Tue, Dec 16, 2025 at 12:33 AM Richard Biener
> > > <[email protected]> wrote:
> > > >
> > > > On Tue, Dec 16, 2025 at 6:36 AM Andrew Pinski
> > > > <[email protected]> wrote:
> > > > >
> > > > > After r16-6104-gb5c64db0a49d46, we try to duplicate bb's
> > > > > that contain loop exit that have "never exit" but we check
> > > > > against the propability of the exit to very_unlikely. If we
> > > > > have PGO, then a loop exit might be very unlikely to be taken
> > > > > if we interate the loop more than 2000 times. So we should take
> > > > > into account if the probability is reliable or not; this basically
> > > > > says if the probability is based on profile data (or corrected
> > > > > profile data)
> > > > > or based on heurstics.
> > > > >
> > > > > This should fix PR 123126 but I don't have a full SPEC setup with PGO
> > > > > to be able to test it.
> > > > >
> > > > > Bootstrapped and tested on x86_64-linux-gnu.
> > > > >
> > > > > gcc/ChangeLog:
> > > > >
> > > > > * tree-ssa-loop-ch.cc (should_duplicate_loop_header_p): Check
> > > > > the reliable_p of the edge's probability.
> > > > >
> > > > > Signed-off-by: Andrew Pinski <[email protected]>
> > > > > ---
> > > > > gcc/tree-ssa-loop-ch.cc | 3 ++-
> > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/gcc/tree-ssa-loop-ch.cc b/gcc/tree-ssa-loop-ch.cc
> > > > > index feecf91cf70..293720fbd04 100644
> > > > > --- a/gcc/tree-ssa-loop-ch.cc
> > > > > +++ b/gcc/tree-ssa-loop-ch.cc
> > > > > @@ -480,7 +480,8 @@ should_duplicate_loop_header_p (basic_block
> > > > > header, class loop *loop,
> > > > > or combining.
> > > > > FIXME: once we have better quality tracking,
> > > > > make this more robust. */
> > > > > - || e->probability <=
> > > > > profile_probability::very_unlikely ()))
> > > > > + || (!e->probability.reliable_p ()
> > > > > + && e->probability <=
> > > > > profile_probability::very_unlikely ())))
> > > >
> > > > Why !e->probability.reliable_p ()? That looks backward.
> > >
> > > `reliable_p ()` says if we are using the profile data from an executed
> > > run rather than the guessed values via heuristics.
> > > `probably_never_executed_edge_p ()` checks the probability that the
> > > edge is never executed (exactly 0%). With profiling data, then the
> > > noreturn edge will be reliable but sometimes with the heuristics the
> > > noreturn/unreachable edges will have a probability less than
> > > very_unlikely.
> > > So in the case with profile data (probability.reliable_p () being
> > > true), we want to only see if it is exactly 0% probability. Rather
> > > than the fuzzy amount of very_unlikely. Because a branch that is taken
> > > less than 1 out of 2000 times (for an example loop with a trip count
> > > over 2000), the code originally would duplicate the header and the
> > > next block too. That is wrong.
> >
> > Yes, which is why I'm suggesting probably_never_executed_edge_p ()
> > as we should predict edges to abort() and the like as executing never.
>
> There is a check on `probably_never_executed_edge_p ()` right above
> already. But I have found (in one of the testcases I added,
> `gcc.dg/tree-ssa/copy-headers-{10,11,12}.c`),
> probably_never_executed_edge_p sometimes returns false for edges to
> abort/trap/unreachable. I didn't look into why though. This is why
> there is an extra check added here.
Ok, I have a fix and we can remove the check for very_unlikely now too.
Honza (in r16-2639-g1d3e713dda99e2) caused to return false in
unlikely_executed_stmt_p for __builtin_unreachable/__builtin_trap and
that caused the very unlikely probability to show up (rather than
never). I suspect it should have been return true (as unlikely to be
executed) and ignore all other heuristics. I am testing that fix and
the removal of the check for very_unlikely in tree-ssa-loop-ch.cc too.
I will also add a testcase for __builtin_abort as I didn't expect
__builtin_trap be slightly different from __builtin_abort.
Thanks,
Andrew
>
> Let me look into the reason why it is not predicted as never then.
>
> Thanks,
> Andrew
>
> >
> > But I'll leave this to Honza.
> >
> > Richard.
> >
> > >
> > > Thanks,
> > > Andrew
> > >
> > > >
> > > > Hopefully probably_never_executed_edge_p will DTRT here? I'll note
> > > > there's also edge_probability_reliable_p () which uses
> > > > .probably_reliable_p ().
> > > >
> > > > The profile API is too convoluted IMO :/ Possibly full of legacy stuff.
> > > >
> > > > Honza?
> > > >
> > > > > {
> > > > > hasone = true;
> > > > > if (dump_file && (dump_flags & TDF_DETAILS))
> > > > > --
> > > > > 2.43.0
> > > > >
