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

Mon Jan 4 23:37:56 PST 2021

fhahn added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8610
+        Sink->removeFromParent();
+        NextBlock->insert(Sink, NextBlock->begin());
+        continue;
----------------
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.

================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:670
+  /// \pre I is a valid iterator into BB.
+  void moveBefore(VPBasicBlock &BB, iplist<VPRecipeBase>::iterator I);
+
----------------
Could you also add a unit test to `VPlanTEst.cpp`?

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

https://reviews.llvm.org/D93629