[PATCH] D93629: [LV] Don't sink into replication regions

Thu Jan 7 05:30:18 PST 2021

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

In D93629#2481794 <https://reviews.llvm.org/D93629#2481794>, @dmgreen wrote:

> Added a unit test, that caught that the Parent was not set correctly.

Great thanks!

LGTM

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8610
+        Sink->removeFromParent();
+        NextBlock->insert(Sink, NextBlock->begin());
+        continue;
----------------
dmgreen wrote:
> fhahn wrote:
> > fhahn wrote:
> > > can this just be `Sink->moveBefore(NextBlock->getFirstInsertionPt())`? With that, `Sink->removeFromParent()` shouldn't be needed?
> > Oh right, `moveBefore` was still missing. Thanks for adding it. I'm not sure if there's a benefit of having the variant that takes a basic block & an iterator? For consistency with the existing functions, I think it would be slightly better to just have the version that moves before a `VPRecipeBase` (same as `moveAfter`)? Those versions are easier & more convenient to use, IMO.
> It was added because the iterator might be end(), in which case it's not possible to tell which block it originally referred to. (The block might be empty, that's why it can't use moveAfter). It is modelled after this equivalent method, from BasicBlock/Instruction: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/IR/Instruction.h#L142
oh right, because there are no terminator instructions in a VPlan BB. That's a bit unfortunate but not much we can do in that case. Thanks

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

https://reviews.llvm.org/D93629