On Fri, Jan 21, 2022 at 1:12 PM Aldy Hernandez <al...@redhat.com> wrote:
>
> On Fri, Jan 21, 2022 at 11:56 AM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Fri, Jan 21, 2022 at 11:30 AM Aldy Hernandez <al...@redhat.com> wrote:
> > >
> > > On Fri, Jan 21, 2022 at 10:43 AM Richard Biener
> > > <richard.guent...@gmail.com> wrote:
> > > >
> > > > On Fri, Jan 21, 2022 at 9:30 AM Aldy Hernandez via Gcc-patches
> > > > <gcc-patches@gcc.gnu.org> wrote:
> > > > >
> > > > > As discussed in PR103721, the problem here is that we are crossing a
> > > > > backedge and causing us to use relations from a previous iteration of
> > > > > a
> > > > > loop.
> > > > >
> > > > > This handles the testcases in both PR103721 and PR104067 which are
> > > > > variants
> > > > > of the same thing.
> > > > >
> > > > > Tested on x86-64 Linux with the usual regstrap as well as verifying
> > > > > the
> > > > > thread count before and after the patch. The number of threads is
> > > > > reduced by a miniscule amount.
> > > > >
> > > > > I assume we need release manager approval at this point? OK for
> > > > > trunk?
> > > >
> > > > Not for regression fixes.
> > >
> > > OK, I've pushed it to fix the P1s. We can continue refining the
> > > solution in a follow-up patch.
> > >
> > > >
> > > > Btw, I wonder whether you have to treat irreducible regions in the same
> > > > way more generally - which edges are marked as backedge there depends
> > > > on which edge into the region was visited first. I also wonder how we
> > >
> > > Jeff, Andrew??
> > >
> > > > I also wonder how we guarantee that all users of the resolve mode have
> > > > backedges marked
> > > > properly? Also note that CFG manipulation routines in general do not
> > > > keep backedge markings up-to-date so incremental modification and
> > > > resolving queries might not mix.
> > > >
> > > > It's a bit unfortunate that we can't query the CFG on whether backedges
> > > > are marked.
> > >
> > > Ughh. The call to mark_dfs_back_edges is currently in the backward
> > > threader. Perhaps we could put it in the path_range_query
> > > constructor? That way other users of path_range_query can benefit
> > > (loop_ch for instance, though it doesn't use it in a way that crosses
> > > backedges so perhaps it's unaffected). Does that sound reasonable?
> >
> > Hmm, I'd rather keep the burden on the callers because many already
> > should have backedges marked. What you could instead do is
> > add sth like
> >
> > if (flag_checking)
> > {
> > auto_edge_flag saved_dfs_back;
> > for-each-edge-in-cfg () set saved_dfs_back flag if dfs_back is
> > set, clear dfs_back
> > mark_dfs_back_edges ();
> > for-each-edge-in-cfg () verify the flags are set on the same
> > edges and clear saved_dfs_back
> > }
> >
> > to the path_range_query constructor. That way we at least notice when
> > passes
> > do _not_ have the backedges marked properly.
>
> Sounds good. Thanks.
>
> I've put the verifier by mark_dfs_back_edges(), since it really has
> nothing to do with the path solver. Perhaps it's useful for someone
> else.
>
> The patch triggered with the loop-ch use, so I've added a
> corresponding mark_dfs_back_edges there.
>
> OK pending tests?
Please rename dfs_back_edges_available_p to sth not suggesting
it's a simple predicate check, like maybe verify_marked_backedges.
+
+ gcc_checking_assert (!m_resolve || dfs_back_edges_available_p ());
I'd prefer the following which allows even release checking builds
to verify this with -fchecking.
if (!m_resolve)
if (flag_checking)
verify_marked_backedges ();
and then ideally verify_marked_backedges () should raise an
internal_error () for the edge not marked properly, best by
also specifying it. Just like other CFG verifiers do.
The loop ch and backwards threader changes are OK. You
can post the verification independently again.
Thanks,
Richard.
>
> Aldy
