On Tue, Feb 1, 2022 at 7:41 PM Aldy Hernandez <al...@redhat.com> wrote:
>
> Ping
I didn't quite get Jeffs comment, so I waited (sorry). I've meanwhile added
extern bool mark_dfs_back_edges (struct function *);
so please make verify_mark_backedges take a struct function * and replace
'cfun' with the explicit argument.
+ gcc_unreachable ();
should be sth like
internal_error ("%<verify_marked_backedges%> failed");
OK with those changes.
Thanks,
Richard.
> 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.
