This is an archive of the discontinued LLVM Phabricator instance.

[IndVarSimplify][SimplifyIndVar] Move WidenIV to Utils/SimplifyIndVar. NFCI.
ClosedPublic

Authored by SjoerdMeijer on Oct 29 2020, 1:49 PM.

Details

Summary

This moves WidenIV from IndVarSimplify to Utils/SimplifyIndVar so that we have createWideIV available as a generic helper utility. I.e., this is not only useful in IndVarSimplify, but could be useful for loop transformations. For example, motivation for this refactoring is the loop flatten transformation: if induction variables in a loop nest can be widened, we can avoid having to perform certain overflow checks, enabling this transformation.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Oct 29 2020, 1:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2020, 1:49 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
SjoerdMeijer requested review of this revision.Oct 29 2020, 1:49 PM

Yeah, this sounds like it would be useful on it's own.

llvm/include/llvm/Transforms/Utils/LoopUtils.h
463 ↗(On Diff #301733)

Is it worth making this a member of WidenIV? It is used in other places, but usually in conjunction with WidenIV I think.

476 ↗(On Diff #301733)

This one seems to only be used WidenIV. Might as well make it a subclass.

llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
638

UsePostIncrementRanges could be set in the constructor of WidenIV. Or the option could be moved to LoopUtils?

Thanks for looking, and comments addressed.

fhahn added inline comments.
llvm/include/llvm/Transforms/Utils/LoopUtils.h
460 ↗(On Diff #302240)

If this must indeed be part of the header, this needs a doc-comment

466 ↗(On Diff #302240)

Does this all need to be part of the header? Or can we hide most of that in looputils.cpp and just expose a single function to do the widening?

I guess the better place for it would be Utils/SimplifyIndVars.h/cpp.

I guess the better place for it would be Utils/SimplifyIndVars.h/cpp.

I am sorry, but I am not following this. This was factored out from IndVarSimplify and moved to LoopUtils, so that I can reuse it in e.g. this loop pass here D90640.

SjoerdMeijer added inline comments.Nov 3 2020, 1:14 AM
llvm/include/llvm/Transforms/Utils/LoopUtils.h
466 ↗(On Diff #302240)

Thanks, I will have a go at exposing one function, and move this stuff to LoopUtils.cpp

This moves the class definition of WidenIV to LoopUtils.cpp, so that we only expose one function in LoopUtils.h: createWideIV().
I have kept one little helper struct, because it is shared and most convenient just to have it there.

dmgreen accepted this revision.Nov 5 2020, 2:22 AM

Good suggestion on a single function. I went through again and made use the diffs are the same. This looks good to me if noone else has any other comments.
LGTM

This revision is now accepted and ready to land.Nov 5 2020, 2:22 AM
nikic added a subscriber: nikic.Nov 5 2020, 2:28 AM

I guess the better place for it would be Utils/SimplifyIndVars.h/cpp.

I am sorry, but I am not following this. This was factored out from IndVarSimplify and moved to LoopUtils, so that I can reuse it in e.g. this loop pass here D90640.

Just to be clear, we both have an IndVarSimplify pass (where you are extracting this code from) and a SimplifyIndVar utility (which is used by IndVarSimplify, but also e.g. LoopUnroll). I believe the suggestion here is to put this into the SimplifyIndVar transformation utility rather than LoopUtils.

Ah. That does sound like a good suggestion.

I guess the better place for it would be Utils/SimplifyIndVars.h/cpp.

I am sorry, but I am not following this. This was factored out from IndVarSimplify and moved to LoopUtils, so that I can reuse it in e.g. this loop pass here D90640.

Just to be clear, we both have an IndVarSimplify pass (where you are extracting this code from) and a SimplifyIndVar utility (which is used by IndVarSimplify, but also e.g. LoopUnroll). I believe the suggestion here is to put this into the SimplifyIndVar transformation utility rather than LoopUtils.

Ah, luckily SimplifyIndVar and IndVarSimplify is not very confusing! ;-)

I think it's arbitrary if this should live in utility SimplifyIndVar or LoopUtils as both seem a good fit. Personally, I would probably put in LoopUtils, but am really happy to move it to SimplifyIndVar if that's what we want.

SjoerdMeijer retitled this revision from [IndVarSimplify][LoopUtils] Move WidenIV to Utils. NFCI. to [IndVarSimplify][SimplifyIndVar] Move WidenIV to Utils/SimplifyIndVar. NFCI..
SjoerdMeijer edited the summary of this revision. (Show Details)

Comments addressed: moved this to Uitls/SimplifyIndVar.

fhahn accepted this revision.Nov 5 2020, 7:08 AM

LGTM thanks for trimming down the API entry point! Utils/SimplifyIndVar.cpp seems a better fit as mentioned earlier.

llvm/include/llvm/Transforms/Utils/SimplifyIndVar.h
19

those includes are not needed?

62

It seems like this comment needs updating. It seems to mentioned some implementation details. Probably better to just state that it contains information about a widened IV or something along those lines.

Thank you guys for reviewing!
Will address the minor comments before committing.