[PATCH] D75980: [LV] Generate RT checks up-front and remove them if required.

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2883-2886
+  auto *PHTerm = Pred->getTerminator();
+  for (unsigned i = 0; i < PHTerm->getNumSuccessors(); i++)
+    if (PHTerm->getSuccessor(i) == LoopVectorPreHeader)
+      PHTerm->setSuccessor(i, SCEVCheckBlock);
----------------
lebedev.ri wrote:
> 
Please address.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:435
 }
-
+struct GeneratedRTChecks;
 namespace llvm {
----------------
Please add empty lines around and add something like "// Forward declarations" or similar


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:460
                       LoopVectorizationCostModel *CM, BlockFrequencyInfo *BFI,
-                      ProfileSummaryInfo *PSI)
+                      ProfileSummaryInfo *PSI, GeneratedRTChecks &Check)
       : OrigLoop(OrigLoop), PSE(PSE), LI(LI), DT(DT), TLI(TLI), TTI(TTI),
----------------
Would "RTChecks" be a better name?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:741
   /// had to make are correct.
-  void emitSCEVChecks(Loop *L, BasicBlock *Bypass);
+  BasicBlock *emitSCEVChecks(Loop *L, BasicBlock *Bypass);
 
----------------
Update description on return value. Also consider using Optional<BasicBlock *>.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1888
+/// vectorize, the checks are moved backed. If deciding not to vectorize, the
+/// temporary block is completely removed.
+struct GeneratedRTChecks {
----------------
Some general notes. I would like us to:
1) Consolidate all the work related to runtime checks under this helper abstraction. Currently, it looks like some functionality still on it's old place. Find more comments on this as you go.
2) Make clear boundaries between this GeneratedRTChecks and its users. Please, convert it to class and define clear public API.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1902
+
+  GeneratedRTChecks(BasicBlock *Preheader, ScalarEvolution &SE,
+                    DominatorTree *DT)
----------------
'Preheader' looks redundant here as it can easily be get from Loop


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1912
+  /// no vector code generation, the check blocks is removed completely.
+  void Create(Loop *L, const LoopAccessInfo &LAI,
+              const SCEVUnionPredicate &UnionPred, LoopInfo *LI) {
----------------
Do you really need as separate hook to kick in generation of runtime checks? In other words can we simply move this functionality to constructor?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1915
+    BasicBlock *LoopHeader = Preheader->getSingleSuccessor();
+    TmpBlock = SplitBlock(Preheader, Preheader->getTerminator(), DT, LI,
+                          nullptr, "tmp.rtchecks");
----------------
The goal is to create "un-linked" block(s). Right? In that case, would it be simpler to not depend on Preheader and use CreateBlock directly. I believe it would be easier to un-link the block then.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1924
+      std::tie(FirstCheckInst, MemRuntimeCheck) = addRuntimeChecks(
+          TmpBlock->getTerminator(), L, RtPtrChecking.getChecks(), Exp);
+      assert(MemRuntimeCheck && "no RT checks generated although RtPtrChecking "
----------------
I would suggest generating SCEV and MemRuntime checks into separate temporary blocks in the first place. I believe that will simplify further managment.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1958
+
+    SmallPtrSet<Value *, 8> Removed;
+    // Completely remove the block.
----------------
I'm surprised to see the following piece of code dealing with deletion. I would expected SCEVExpanderCleaner does all necessary cleanup. It should be enough just to properly set "markResultUsed". If this is not the case can we rework in this direction?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3282
   // Create new preheader for vector loop.
-  LoopVectorPreHeader =
-      SplitBlock(SCEVCheckBlock, SCEVCheckBlock->getTerminator(), DT, LI,
-                 nullptr, "vector.ph");
+  Check.TmpBlock = SplitBlock(
+      SCEVCheckBlock, cast<Instruction>(Check.SCEVCheck)->getNextNode(),
----------------
I believe this functionality should be moved to GeneratedRTChecks.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3289-3290
+
+  Check.TmpBlock->replaceAllUsesWith(SCEVCheckBlock);
+  Check.TmpBlock->getTerminator()->moveBefore(SCEVCheckBlock->getTerminator());
+  SCEVCheckBlock->getTerminator()->eraseFromParent();
----------------
I believe this functionality should be moved to GeneratedRTChecks.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3357
+  DT->changeImmediateDominator(LoopVectorPreHeader, MemCheckBlock);
+  Check.TmpBlock->moveBefore(LoopVectorPreHeader);
+
----------------
I believe this functionality should be moved to GeneratedRTChecks.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3375
   }
+  Check.TmpBlock = nullptr;
 
----------------
TmpBlock should not be directly exposed outside of GeneratedRTChecks.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8056
   // expressions.
-  BasicBlock *SavedPreHeader = LoopVectorPreHeader;
-  emitSCEVChecks(Lp, LoopScalarPreHeader);
-
-  // If a safety check was generated save it.
-  if (SavedPreHeader != LoopVectorPreHeader)
-    EPI.SCEVSafetyCheck = SavedPreHeader;
+  EPI.SCEVSafetyCheck = emitSCEVChecks(Lp, LoopScalarPreHeader);
 
----------------
I think we should better get all info about runtime checks from GeneratedruntimeChecks directly.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9519
+  {
+    GeneratedRTChecks Checks(L->getLoopPreheader(), *PSE.getSE(), DT);
+    InnerLoopVectorizer LB(L, PSE, LI, DT, TLI, TTI, AC, ORE, VF.Width, 1, LVL,
----------------
Why do we need to create GeneratedRTChecks  outside of InnerLoopVectorizer? Can/Should it have bigger life time?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9768
 
-  LVP.setBestPlan(VF.Width, IC);
+  if (Requirements.doesNotMeet(F, L, Hints)) {
+    LLVM_DEBUG(dbgs() << "LV: Not vectorizing: loop did not meet vectorization "
----------------
Why this was moved from its original place?


================
Comment at: llvm/test/Transforms/LoopVectorize/pr47343-expander-lcssa-after-cfg-update.ll:59
 ; CHECK:       scalar.ph:
-; CHECK-NEXT:    [[TMP4:%.*]] = phi i8* [ [[TMP1]], %vector.memcheck ], [ [[TMP1]], %loop.preheader ], [ [[TMP1]], %middle.block ]
 ; CHECK-NEXT:    [[BC_RESUME_VAL:%.*]] = phi i32 [ 500, %middle.block ], [ 0, %loop.preheader ], [ 0, %vector.memcheck ]
----------------
Looks like we stopped vectorizing something, did we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75980