This is an archive of the discontinued LLVM Phabricator instance.

[NFC] [LoopUtil] Moved RecurrenceDescriptor/LoopDescriptor from Transform/Utils/LoopUtils.* to Analysis tree
AbandonedPublic

Authored by hsaito on Apr 8 2018, 2:16 PM.

Details

Summary

I'd like to move LoopVectorizationLegality in Transform/Vectorize/LoopVectorize.cpp to Analysis tree so that it is reusable from outside of loop vectorizer (e.g., other analysis/transformation to check whether loop is vectorizable and behave differently).
RecurrenceDescriptor and InductionDescriptor classes are used by LoopVectorizationLegality class. Nature of all those classes are indeed
analysis and thus Analysis tree is a better place to house them.

Includes and forward declared classes are also cleaned up. I really didn't want to include IRBuilder.h from a header --- since it includes a lot of other headers, but it looked like forward declaring IRBuilder wasn't too appealing to me.

Diff Detail

Event Timeline

hsaito created this revision.Apr 8 2018, 2:16 PM
hsaito added a comment.Apr 8 2018, 2:43 PM

This is loosely related to D45072. After these cleanup, I'll move LoopVectorizationLegality out of LoopVectorize.cpp into Analysis tree.

hsaito retitled this revision from Moved RecurrenceDescriptor/LoopDescriptor from Transform/Utils/LoopUtils.* to Analysis tree to [NFC] [LoopUtil] Moved RecurrenceDescriptor/LoopDescriptor from Transform/Utils/LoopUtils.* to Analysis tree.

Please see D45552 (LoopVectorizationLegality.h) for why this patch is needed for that patch. We can't make Analysis code (i.e., LoopVectorizationLegality in this context) dependent on TransformUtils (Descriptor code w/o this patch).

Eugene.Zelenko added inline comments.Apr 12 2018, 6:31 AM
include/llvm/Analysis/Utils/LoopUtils.h
84

You could use default member initialization for MinMaxKind.

rengolin added inline comments.
lib/Transforms/Scalar/LICM.cpp
46

Why these new includes?

hsaito added inline comments.Apr 13 2018, 12:16 PM
lib/Transforms/Scalar/LICM.cpp
46

Moved #include from .h to .cpp. Header file doesn't really need to #include. it Forward declaration of class names are enough. Just followed "#include as little as possible" when I tried to figure out which #includes are actually needed.

hsaito added inline comments.Apr 13 2018, 12:26 PM
include/llvm/Analysis/Utils/LoopUtils.h
84

OK. I can include that change.

So, I never liked LoopUtils and I agree a lot of it should go to Analysis but having two copies of LoopUtils is bound to confuse people when including the headers.

Induction and reduction analysis is very core to the vectoriser, that's why it's always been there. I'd prefer a more specific approach like naming it InductionAnalysis or RecurrenceAnalysis and only moving those parts out to Analysis.

Being this a higher level change, I'd like more people to have a look at it. Adding them as reviewers.

cheers
--renato

lib/Transforms/Scalar/LICM.cpp
46

Ah, makes sense, thanks!

Thanks, Renato for the feedback.

So, I never liked LoopUtils and I agree a lot of it should go to Analysis but having two copies of LoopUtils is bound to confuse people when including the headers.

I fully agree with you, and I actually expected someone to bring it up. On the other hand, my main motivation was to bring those two classes to Analysis and much less on how I name files, I decided to wait for someone else to speak up for the file naming issues.

Induction and reduction analysis is very core to the vectoriser, that's why it's always been there. I'd prefer a more specific approach like naming it InductionAnalysis or RecurrenceAnalysis and only moving those parts out to Analysis.

I can further split this into InductionAnalysis.h/.cpp and RecurrenceAnalysis.h/.cpp, instead of LoopUtils.h/.cpp ----- if they are the names we'd like to settle. I believe I moved only the class member functions and the static file scope functions called from the member functions. If you think those static file scope functions should be moved into private static functions of the class, I can do that as a separate patch. I'd rather keep this patch as a plain "move".

Adding Diego and Florian. We just had a good chat about this and maybe keeping the headers inside Transform for now would be better for the changes to come. We can move it later when it becomes clearer what the purpose is.

We can also split LoopUtils while still inside Transform and then move to Analysis later when it makes sense.

Adding Diego and Florian. We just had a good chat about this and maybe keeping the headers inside Transform for now would be better for the changes to come. We can move it later when it becomes clearer what the purpose is.

We can also split LoopUtils while still inside Transform and then move to Analysis later when it makes sense.

Side effect of that is D45552 will have to land in Transform first and then move to Analysis when the descriptors move.

what the purpose is.

For me, placing the code in the right location is one of the simplest things to avoid duplicate efforts and improve reusability. Having said that, our near-term reuse of Legal (D45552) change still works even if it lands in Transform. So, I won't insist like a purist. Then, I no longer have a good motivation to split the LoopUtil, but since I don't like to do the same thing twice, I might as well finish the splitting work.

So, at the moment, what we need to settle are 1) file names to use and 2) move to Analysis or keep it under Transform.

Thanks,
Hideki

Side effect of that is D45552 will have to land in Transform first and then move to Analysis when the descriptors move.

Should be fine.

For me, placing the code in the right location is one of the simplest things to avoid duplicate efforts and improve reusability. Having said that, our near-term reuse of Legal (D45552) change still works even if it lands in Transform. So, I won't insist like a purist. Then, I no longer have a good motivation to split the LoopUtil, but since I don't like to do the same thing twice, I might as well finish the splitting work.

I see. Usually, when we move code, we make sure that the use case for being in a more generic part of the tree has a definite improvement, not just being a logical move.

This is mostly because we want to keep the code that is only used for transforms inside its own directory, even if it's an "analysis" per se.

A strong case for the move would be having another part of the code, not transform, needing that analysis. Once we have one, then move becomes obvious.

So, at the moment, what we need to settle are 1) file names to use and 2) move to Analysis or keep it under Transform.

We should keep inside Transform for now, until we have non-Transform code using it.

I agree not splitting LoopUtils is probably the simplest way forward. I don't like it as well, but I'd like to avoid code motion that can move again once we have a clear view, making it hard to git bisect changes (especially in a time of VPlan potentially breaking the world. :)

cheers,
--renato

So, at the moment, what we need to settle are 1) file names to use and 2) move to Analysis or keep it under Transform.

We should keep inside Transform for now, until we have non-Transform code using it.

I agree not splitting LoopUtils is probably the simplest way forward. I don't like it as well, but I'd like to avoid code motion that can move again once we have a clear view, making it hard to git bisect changes (especially in a time of VPlan potentially breaking the world. :)

OK. I'll let Legal change land on Transform and postpone the split of LoopUtils. As a result, this patch will be abandoned. FYI, next thing I'll be working on is LoopVectorizationCostModel ---- to make it work on VPlan.

hsaito abandoned this revision.Apr 26 2018, 12:38 PM

I may come back with the same change at some later time, but this patch is abandoned for the time being, as discussed above.