Ping
On Mon, Jan 24, 2022, 20:20 Aldy Hernandez <al...@redhat.com> wrote:
> On Mon, Jan 24, 2022 at 9:51 AM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > 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.
>
> How about this?
>
> Tested on x86-64 Linux.
>
