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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 11 inline comments as done.
fhahn added inline comments.


================
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))) {
----------------
ebrevnov wrote:
> Please revisit
Fixed comment, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2042
+        BranchInst::Create(Bypass, LoopVectorPreHeader, SCEVCheck));
+    SCEVCheck = nullptr;
+    return SCEVCheckBlock;
----------------
ebrevnov wrote:
> Why do we need to set it to null?
This is used to indicate the check was used and should not be cleaned up. I'll add a comment.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2069
+
+    auto *C = MemCheckBlock;
+    MemRuntimeCheck = nullptr;
----------------
ebrevnov wrote:
> Why storing to temp?
> 
It's not needed any longer, earlier versions set `MemCheckBlock = nullptr`. I removed it.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2070
+    auto *C = MemCheckBlock;
+    MemRuntimeCheck = nullptr;
+    return C;
----------------
ebrevnov wrote:
> Why needed?
This is used to indicate the check was used and should not be cleaned up. I'll add a comment.



================
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) {
----------------
ebrevnov wrote:
> 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?
I forgot to post my response here. In some cases we know that we do not need runtime checks up front (e.g. because we only interleave the loop). Having a helper to only generate the RT checks if needed helps to avoid unnecessary work.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1915
+    BasicBlock *LoopHeader = Preheader->getSingleSuccessor();
+    TmpBlock = SplitBlock(Preheader, Preheader->getTerminator(), DT, LI,
+                          nullptr, "tmp.rtchecks");
----------------
ebrevnov wrote:
> 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?
That's a great idea, I added the comment. All instructions that are generated by SCEVExpander should be tracked by it; it uses an IRBuilder callback to collect them, so it should not matter where exactly they are added.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1623
+
+    if (MemRuntimeCheck && !isa<Constant>(MemRuntimeCheck))
+      MemRuntimeCheck->replaceAllUsesWith(
----------------
lebedev.ri wrote:
> I'm guessing it can't be `i1 true` ?
I am not entirely sure, but I think if the compares could be simplified to `true`/`false` during generation from SCEV expression, LoopAccessInfo should have been able to figure out that the checks are unneeded. We could also check for `i1 true`, but I think that would be best in a separate patch, as the original code had the same assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75980