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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8511
+    // If the target is in a replication region, make sure to move Sink to the
+    // block after it, not into the replication region itself.
+    if (auto *Region =
----------------
Ayal wrote:
> Thanks for fixing!
> Note that in general, sinking an instruction more than originally asked for, might place it after its use; in this case, if its use were to belong to the same replication region - potentially possible in the future.
> Is it possible for NextBlock to be another replicating Region rather than a BasicBlock?
> Going forward, such legality-based scheduling motions should probably be applied earlier, to an initial Region-free VF=UF=1 VPlan.
> Is it possible for NextBlock to be another replicating Region rather than a BasicBlock?

No, Not currently. There is always an empty VPBasicBlock between one replication region and the next. That is what sink_into_replication_region_multiple is testing, along with the two replication regions not being combined into a single block. We can't (currently) sink past the wrong instruction, as far as I understand, because the replication region is not combined into anything else and we are only sinking past that region, not anything else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93629