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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added a comment.

Thanks for doing the changes. Overall this version looks fine to me. Please go ahead with some final cleanups.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:748
   /// Emit bypass checks to check any memory assumptions we may have made.
-  void emitMemRuntimeChecks(Loop *L, BasicBlock *Bypass);
+  BasicBlock *emitMemRuntimeChecks(Loop *L, BasicBlock *Bypass);
 
----------------
Please update this one as well.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1996-1997
+      // Memory runtime check generation creates compares that use expanded
+      // values. Remove them before running the clerunning the clerunning the
+      // cleaner.
+      for (auto &I : make_early_inc_range(reverse(*MemCheckBlock))) {
----------------
Please revisit


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2042
+        BranchInst::Create(Bypass, LoopVectorPreHeader, SCEVCheck));
+    SCEVCheck = nullptr;
+    return SCEVCheckBlock;
----------------
Why do we need to set it to null?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2069
+
+    auto *C = MemCheckBlock;
+    MemRuntimeCheck = nullptr;
----------------
Why storing to temp?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2070
+    auto *C = MemCheckBlock;
+    MemRuntimeCheck = nullptr;
+    return C;
----------------
Why needed?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1915
+    BasicBlock *LoopHeader = Preheader->getSingleSuccessor();
+    TmpBlock = SplitBlock(Preheader, Preheader->getTerminator(), DT, LI,
+                          nullptr, "tmp.rtchecks");
----------------
fhahn wrote:
> ebrevnov wrote:
> > 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.
> I updated the code put SCEV and memory checks in separate blocks. It still uses `SplitBlock`, because this keeps the blocks in the DT/LI, which may be used during SCEV expansion and it also keeps them linked to the right predecessors during expansion (SCEVExpander tries to find values to re-use in predecessors).
Please, put that info into the code as a comment. SCEVExpander also can decide to hoist some instruction(s) into predecessors. Will they be properly scheduled for deletion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75980