This is an archive of the discontinued LLVM Phabricator instance.

[LoopInfo] Extend getExitEdges API
AbandonedPublic

Authored by skatkov on Jul 1 2019, 9:53 PM.

Details

Reviewers
reames
fhahn
Summary

Add an ability to provide an output argument to store loop exit edges
with pair of const and non-const BasicBlocks.

Diff Detail

Event Timeline

skatkov created this revision.Jul 1 2019, 9:53 PM
fhahn added inline comments.Jul 3 2019, 4:49 AM
include/llvm/Analysis/LoopInfo.h
282

Is there a reason why the implementation was moved to here? Could it stay in LoopInfoImpl.h?

skatkov marked an inline comment as done.Jul 3 2019, 8:00 PM
skatkov added inline comments.
include/llvm/Analysis/LoopInfo.h
282

Yes, there is a reason. To support both const and non-const ParamBlockT in SmallVector argument I introduced the new template parameter ParamBockT. As a result if I keep the implementation in LoopInfoImpl.h I have a compilation error due to users includes this header. So the movement to this header is required.

fhahn added inline comments.Jul 5 2019, 2:58 AM
include/llvm/Analysis/LoopInfo.h
282

On second thought, it looks like getExitEdges has only a single user, so maybe it would be better to just change the original API from const to non-const and update LoopPredication?

This would be in line with most of the other APIs, that already return non-const BlockT*, exactly for the reason that most APIs expect non-const blocks.

skatkov marked an inline comment as done.Jul 5 2019, 3:04 AM
skatkov added inline comments.
include/llvm/Analysis/LoopInfo.h
282

I can do this.
I just wonder what wrong with movement of this code from LoopInfoImpl.h to LoopInfo.h and support both const and non-const cases?

fhahn added inline comments.Jul 5 2019, 3:19 AM
include/llvm/Analysis/LoopInfo.h
282

Nothing per-se, IMO it is just slightly preferable to keep the changes as simple as possible/required; and given most other functions in LoopBase already return non-const, it seems reasonable to bring getExitEdges in line.

(Potential drawbacks are increased compile-time/size, as LoopInfo.h is widely used and it adds an additional step to git-blame)

reames requested changes to this revision.Jul 8 2019, 10:05 AM

Given all the other APIs on this class work in terms of BlockT*, not const BlockT*, I think it would be more consistent to just change the definition of Edge in this case. The one caller should be trivial to update.

In theory, being const correct is better, but consistency in the code overrides to me.

If you want to make the suggested change to Edge, feel free to submit without further review.

This revision now requires changes to proceed.Jul 8 2019, 10:05 AM