[PATCH] D101290: [LV] Try to sink and hoist inside candidate loops for vectorization.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9989
+  bool Changed = false;
+  DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Eager);
+  for (BasicBlock *BB : make_early_inc_range(L->blocks())) {
----------------
lebedev.ri wrote:
> FWIW these sink/hoist helpers are fine with lazy updates.
I'm not sure what the issue is exactly, but I think there are some cases where combining sinking & hoisting could result in some duplicated/outdated updates which would require `applyUpdatesPermissive`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10006-10016
+      if (hoistThenElseCodeToIf(BI, TTI, false, &DTU)) {
+        Changed = true;
+        SmallPtrSet<BasicBlock *, 2> NewSuccs(succ_begin(BB), succ_end(BB));
+        if (!NewSuccs.count(OldThen)) {
+          LI.removeBlock(OldThen);
+          DeleteDeadBlock(OldThen);
+        }
----------------
lebedev.ri wrote:
> I think it would be fine to sink that into `hoistThenElseCodeToIf()`.
Unfortunately I think that would be incompatible with the use in SimplifyCFG, which does not seem to handle removing BBs other than the current one well.

But I adjusted the loop to remove any blocks that became dead at the top-level, which hopefully makes things a bit clearer.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10025-10031
+      if (sinkCommonCodeFromPredecessors(BB, &DTU)) {
+        // sinkCommonCodeFromPredecessors may introduce a new split block in
+        // the loop. Add it.
+        for (auto *Pred : predecessors(BB)) {
+          if (!L->contains(Pred))
+            L->addBasicBlockToLoop(Pred, LI);
+        }
----------------
lebedev.ri wrote:
> I think it would be fine to sink that into `sinkCommonCodeFromPredecessors()`.
> 
Thanks, that's a great idea. I put up D101368 to optionally preserve LI in `sinkCommonCodeFromPredecessors`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101290