This is an archive of the discontinued LLVM Phabricator instance.

[LCG] Add utilities to compute parent and ascestor relationships between SCCs.
ClosedPublic

Authored by chandlerc on Oct 12 2016, 2:31 AM.

Details

Summary

These will be fairly expensive routines to call and might be abused in
real code, but are quite useful when debugging or in asserts and are
reasonable and well formed properties to query.

I've used one of them in an assert that was requested in a code review
here.

My real question: are these utilities worth adding now that we see the
amount of code involved? I just want to double check that this is what
was expected when it was brought up in code review.

Provided this is what we want, I'll add more comprehensive coverage of
these routines in the unittests to make sure they actually work
correctly, etc. Just wanted the sanity check first.

Event Timeline

chandlerc updated this revision to Diff 74346.Oct 12 2016, 2:31 AM
chandlerc retitled this revision from to [LCG] Add utilities to compute parent and ascestor relationships between SCCs..
chandlerc updated this object.
chandlerc added a reviewer: sanjoy.
chandlerc added a subscriber: llvm-commits.
sanjoy requested changes to this revision.Oct 15 2016, 3:37 PM
sanjoy edited edge metadata.
sanjoy added a subscriber: mzolotukhin.

I'm in favor of adding this -- IME verification like this has always paid off (recent examples include the LCSSA verification that @mzolotukhin has been working on).

I do have one comment inline.

lib/Analysis/LazyCallGraph.cpp
212

Why not walk over SCC instances directly? The way you've written these, I think you'll spend time "re-discovering" the SCC structures (that is walking intra-SCC call edges) when that is avoidable.

This revision now requires changes to proceed.Oct 15 2016, 3:37 PM
reames added a subscriber: reames.Oct 20 2016, 5:58 PM

I'm in favor of adding this -- IME verification like this has always paid off (recent examples include the LCSSA verification that @mzolotukhin has been working on).

I'm also in support for the same reasons.

chandlerc updated this revision to Diff 78835.Nov 21 2016, 11:28 PM
chandlerc edited edge metadata.

Address Sanjoy's comment.

chandlerc added inline comments.Nov 21 2016, 11:29 PM
lib/Analysis/LazyCallGraph.cpp
212

Good idea. I was thinking that it wouldn't be any better to do this because we have to walk the nodes anyways, but you're totally right this is much more efficient if it collapses immediately to SCCs. Please have a look and make sure this matches what you expected.

sanjoy accepted this revision.Nov 22 2016, 4:23 AM
sanjoy edited edge metadata.

lgtm

lib/Analysis/LazyCallGraph.cpp
1409

I'd drop the "potentially" -- not sure what that means here.

This revision is now accepted and ready to land.Nov 22 2016, 4:23 AM
This revision was automatically updated to reflect the committed changes.