[PATCH] D71055: [LV][NFC] Some refactoring and renaming to facilitate next change.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

> 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.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2687
   Value *Count = getOrCreateTripCount(L);
-  BasicBlock *BB = L->getLoopPreheader();
-  IRBuilder<> Builder(BB->getTerminator());
+  BasicBlock *const OrigPreHeader = LoopVectorPreHeader;
+  IRBuilder<> Builder(OrigPreHeader->getTerminator());
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2696
+  auto P =
+      Cost->requiresScalarEpilogue() ? ICmpInst::ICMP_ULE : ICmpInst::ICMP_ULT;
 
----------------
Unrelated clang-format changes should be kept separate, possibly in another patch.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2704
         "min.iters.check");
 
+  LoopVectorPreHeader = OrigPreHeader->splitBasicBlock(
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2720
 void InnerLoopVectorizer::emitSCEVChecks(Loop *L, BasicBlock *Bypass) {
-  BasicBlock *BB = L->getLoopPreheader();
+  BasicBlock *const OrigPreHeader = LoopVectorPreHeader;
 
----------------
ditto wrt OrigVectorPreHeader or reusing LoopVectorPreHeader.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2737
 
   // Create a new block containing the stride check.
+  OrigPreHeader->setName("vector.scevcheck");
----------------
Such a comment was suggested above.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2759
 
-  BasicBlock *BB = L->getLoopPreheader();
+  BasicBlock *const OrigPreHeader = L->getLoopPreheader();
 
----------------
ditto wrt OrigVectorPreHeader or reusing LoopVectorPreHeader.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3086
   // Get ready to start creating new instructions into the vectorized body.
-  Builder.SetInsertPoint(&*VecBody->getFirstInsertionPt());
-
-  // Save the state.
-  LoopVectorPreHeader = Lp->getLoopPreheader();
-  LoopScalarPreHeader = ScalarPH;
-  LoopMiddleBlock = MiddleBlock;
-  LoopExitBlock = ExitBlock;
-  LoopVectorBody = VecBody;
-  LoopScalarBody = OldBasicBlock;
+  Builder.SetInsertPoint(&*LoopVectorBody->getFirstInsertionPt());
 
----------------
Worth adding an "assert(LoopVectorPreHeader == Lp->getLoopPreheader() && ...);" at some point after filling Lp?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3090
+      makeFollowupLoopID(OrigLoopID, { LLVMLoopVectorizeFollowupAll,
+                                       LLVMLoopVectorizeFollowupVectorized });
   if (VectorizedLoopID.hasValue()) {
----------------
Unrelated clang-format changes should be kept separate, possibly in another patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71055/new/

https://reviews.llvm.org/D71055