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

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
346

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

356

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
346

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

356

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

Don't repeat function names in new doxygen comments.

176

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

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.