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.
Details
Diff Detail
Event Timeline
Yeah, this sounds like it would be useful on it's own.
llvm/include/llvm/Transforms/Utils/LoopUtils.h | ||
---|---|---|
463 | Is it worth making this a member of WidenIV? It is used in other places, but usually in conjunction with WidenIV I think. | |
476 | 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? |
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.
llvm/include/llvm/Transforms/Utils/LoopUtils.h | ||
---|---|---|
466 | 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.
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
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.
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 ↗ | (On Diff #303113) | those includes are not needed? |
62 ↗ | (On Diff #303113) | 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. |
If this must indeed be part of the header, this needs a doc-comment