Page MenuHomePhabricator

[LV][NFC] Some refactoring and renaming to facilitate next change.
ClosedPublic

Authored by ebrevnov on Dec 5 2019, 3:32 AM.

Details

Summary

This refactoring is motivated by target change https://reviews.llvm.org/D71047. See parent/child dependencies for full list.

Currently 'createVectorizedLoopSkeleton' keeps track of new generated blocks using local variables and reassigns them to corresponding class fields at the end. That makes a bit harder to understand code structure. This patch uses class fields directly and extra locals got removed.

Event Timeline

ebrevnov created this revision.Dec 5 2019, 3:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2019, 3:32 AM
ebrevnov updated this revision to Diff 234219.Dec 16 2019, 11:44 PM

Minor update.

ebrevnov edited the summary of this revision. (Show Details)Dec 16 2019, 11:58 PM

Ping

Ayal added a comment.Dec 23 2019, 1:30 PM

Currently 'createVectorizedLoopSkeleton' keeps track of new generated blocks using local variables and reassigns them to corresponding class fields at the end. That makes a bit harder to understand code structure. This patch uses class fields directly and extra locals got removed.

OK. Note that to make the code structure easier to understand, one could alternatively simply rename local variables to "Tmp<class field name>". Local variables may be more efficiently accessed than the class fields they copy, but the former's caching effect may be unclear.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2680–2681

BB is indeed too generic. Note that there are two pre-headers, how about OrigPreHeader >> OrigVectorPreHeader? But it's unclear why LoopVectorPreHeader needs to be copied at all at this point.

2689

Unrelated clang-format changes should be kept separate, possibly in another patch.

2697

The previous "NewBB, BB" emphasized that the pre-header is being renewed. Now when this is done by redefining LoopVectorPreHeader, worth adding a preceding comment here.

Alternatively, use LoopVectorPreHeader above instead of Orig[Vector]PreHeader, and introduce OrigVectorPreHeader here when splitting it and redefining LoopVectorPreHeader.

2713

ditto wrt OrigVectorPreHeader or reusing LoopVectorPreHeader.

2730–2731

Such a comment was suggested above.

2752

ditto wrt OrigVectorPreHeader or reusing LoopVectorPreHeader.

3079

Worth adding an "assert(LoopVectorPreHeader == Lp->getLoopPreheader() && ...);" at some point after filling Lp?

3083

Unrelated clang-format changes should be kept separate, possibly in another patch.

ebrevnov marked 8 inline comments as done.Dec 25 2019, 2:21 AM
ebrevnov added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2730–2731

Actually this comment is wrong.

ebrevnov updated this revision to Diff 235266.Dec 25 2019, 2:22 AM
ebrevnov edited the summary of this revision. (Show Details)

Updated according to comments.

Ayal accepted this revision.Dec 26 2019, 10:36 AM

This looks good to me, with a couple of final optional comments.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2678

Should the "local" Bypass also be removed/replaced, in this and following emit*() methods?

(May have been better if these emit*() methods originally returned the new preheader they create, so their callers would do LoopVectorPreHeader = emit*(Lp, LoopVectorPreHeader), emphasizing the chaining.)

2944

The above 5 lines could remain in their original place; they're not part of splitting the single block loop etc.

This revision is now accepted and ready to land.Dec 26 2019, 10:36 AM
ebrevnov closed this revision.Dec 30 2019, 4:24 AM

ebrevnov committed rG1b6286b945a5: [LV][NFC] Some refactoring and renaming to facilitate next change. (authored by ebrevnov).