This is an archive of the discontinued LLVM Phabricator instance.

Utility to print all the basic blocks of a loop.
ClosedPublic

Authored by hiraditya on Jul 26 2016, 9:14 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hiraditya updated this revision to Diff 65541.Jul 26 2016, 9:14 AM
hiraditya retitled this revision from to Utility to print all the basic blocks of a loop..
hiraditya updated this object.
hiraditya added reviewers: hfinkel, sanjoy.
sanjoy requested changes to this revision.Jul 26 2016, 11:07 AM
sanjoy edited edge metadata.
sanjoy added inline comments.
llvm/include/llvm/Analysis/LoopInfoImpl.h
339 ↗(On Diff #65541)

Not sure if this needs to be a completely separate method -- why not just pass in a verbose flag to print?

349 ↗(On Diff #65541)

Hm, just noticed that this is misleading both here and in the print method above -- if the loop has multiple latches, we won't tag any blocks as <latch>. This should be fixed (we need a isLoopLatch helper like we have isLoopExiting). @hiraditya - do you mind fixing this in a later change? If you won't be able to get to it, please let me know and I'll do it.

This revision now requires changes to proceed.Jul 26 2016, 11:07 AM
hiraditya added inline comments.Jul 26 2016, 11:15 AM
llvm/include/llvm/Analysis/LoopInfoImpl.h
339 ↗(On Diff #65541)

Okay, I'll push the patch with just another parameter to print.

349 ↗(On Diff #65541)

I can do that in a separate patch. Thanks for the pointer.

hiraditya updated this revision to Diff 65579.Jul 26 2016, 1:11 PM
hiraditya edited edge metadata.

Addresssed comments of Sanjoy.

hiraditya marked 2 inline comments as done.Jul 26 2016, 1:12 PM
sanjoy accepted this revision.Jul 26 2016, 1:21 PM
sanjoy edited edge metadata.

LGTM with comments inline.

However: if it isn't too much extra work, can I ask you to please land
the Verbose flag in a separate change from the isLoopLatch change?
If that's too much extra work, don't bother, since these are low risk
changes.

llvm/include/llvm/Analysis/LoopInfo.h
171 ↗(On Diff #65579)

Don't repeat function names in new doxygen comments.

176 ↗(On Diff #65579)

I'd do this slightly differently. I'd have the precondition be that
BB belongs to the loop (and start with an assert(contains(BB))),
and have the body just be a:

auto PredBegin = ...;
auto PredEnd = ...;
return std::find(BB, PredBegin, PredEnd) != PredEnd;
llvm/lib/Analysis/LoopInfo.cpp
392 ↗(On Diff #65579)

Add a /*Verbose=*/ before true and /*Depth=*/ before 0.

This revision is now accepted and ready to land.Jul 26 2016, 1:21 PM
This revision was automatically updated to reflect the committed changes.

LGTM with comments inline.

However: if it isn't too much extra work, can I ask you to please land
the Verbose flag in a separate change from the isLoopLatch change?
If that's too much extra work, don't bother, since these are low risk
changes.

Split in two patches and addressed all last comments:
https://reviews.llvm.org/rL276837
https://reviews.llvm.org/rL276838

Thanks Sanjoy for your review.