[PATCH] D103514: [LV] Support sinking recipe in replicate region after another region.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Minor comments and suggestions. This should compete the handling of sinking a recipe after another, thanks!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9200
+      assert(Target->getParent()->size() == 1 &&
+             "Target must be in a replicator with a single recipe");
+
----------------
These asserts are better placed inside the lambda?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9203
+      if (auto *SinkRegion = GetReplicateRegion(Sink)) {
+        // If the recipe to sink is also in a recpliate region, move the sink
+        // region after the target region.
----------------
`// If the recipe` >> `// The recipe`

"recpliate"


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9209
+        assert(Sink->getParent()->size() == 1 &&
+               "Sink must be in a replicator with a single recipe");
+
----------------
same asserts...


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9222
+      } else {
+        // 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 the target` >> `// The target`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9232
 
     auto *SinkRegion = GetReplicateRegion(Sink);
     // Unless the sink source is in a replicate region, sink the recipe
----------------
Set SinkRegion and TargetRegion once at the outset?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9235
     // directly.
     if (!SinkRegion) {
       Sink->moveAfter(Target);
----------------
Have this simplest and most common (!SinkRegion && !TargetRegion) case early-exit first?
Followed by the next simple (!SinkRegion && TargetRegion) case; both move the Sink recipe via Sink->move*();
Last deal with SinkRegion by first disconnecting it and then placing it between an AfterBlock and its single successor, where AfterBlock is produced by splitting the Target block if !TargetRegion.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9244
     assert(Sink->getParent()->size() == 1 &&
            "parent must be a replicator with a single recipe");
     auto *SplitBlock =
----------------
same assert...


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9252
     VPBlockUtils::disconnectBlocks(SinkRegion, Succ);
     VPBlockUtils::connectBlocks(Pred, Succ);
 
----------------
This part of disconnecting SinkRegion could be done once for both TargetRegion and !TargetRegion cases.


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-sink-replicate-region.ll:388
+  %rem.div = sdiv i32 20, %rem
+  %recur.next = sext i8 %y to i32
+  %gep = getelementptr i32, i32* %ptr, i32 %iv
----------------
%rem and %rem.div both need to sink after Previous = %recur.next (which is invariant...), and do so by first having %rem move after Previous and then having %rem.div move after %rem, where this last move is the one that moves a replicated region after another, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103514