[PATCH] D123537: [VPlan] Model first exit values using VPLiveOut.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Looks good to me! Adding minor suggestions.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:591
-  /// already in place, and we exit the vector loop exclusively to the middle
-  /// block.
-  void fixLCSSAPHIs(VPTransformState &State);
----------------
Worth retaining this explanation when documenting LiveOut/fixPhi()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3394
       PHI->addIncoming(I.second, MiddleBlock);
+    Plan.removeLiveOut(PHI);
   }
----------------
Suffice to remove LiveOut only when actually fixing the incoming, i.e., under the if?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3709
+  } else {
+    // Otherwise all exit values should be forwarded to the scalar loop earlier.
+    Plan.clearLiveOuts();
----------------
I.e., there’s nothing to fix from vector loop, phi should have incoming from scalar loop only?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3710
+    // Otherwise all exit values should be forwarded to the scalar loop earlier.
+    Plan.clearLiveOuts();
   }
----------------
Worth reversing the condition to handle the simpler case first?

Or have  addUsersInExitBlock() bail out early if requiresScalarEpilogue()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3715
+  // in the exit block, so update the builder.
+  State.Builder.SetInsertPoint(State.CFG.ExitBB, State.CFG.ExitBB->begin());
+  for (auto &KV : Plan.getLiveOuts())
----------------
Set to first non phi instead of begin, as we prepare for non phi extracts, although middle block is free of (LCSSA) phi’s atm?

May as well pass MiddleBlock to fixPhi, having directed Builder to it, instead of having each fixPhi derive it from Plan? (Can also have fixPhi derive MiddleBlock from Builder…)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3870
+        State.Plan->removeLiveOut(&LCSSAPhi);
+      }
 }
----------------
Single else placed earlier suffices, no need for same else here as well?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3928
       // and so use the select value for the phi instead of the old
-      // LoopExitValue.
+      // LoopLiveOut.
       if (PreferPredicatedReductionSelect ||
----------------
In general LiveOut exits from VPlan to IR rather than from Loop. Distinction more important for outerloop vectorization.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4056
+        State.Plan->removeLiveOut(&LCSSAPhi);
+      }
 
----------------
A bit confusing when to else cleanup LiveOuts; perhaps do so directly rather than as an else, or when creating LiveOuts?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8717
+  // Only handle single-exit loops for now.
+  if (!ExitBB->getSinglePredecessor())
+    return;
----------------
Can fold both early exits into one.

Also early exit if requiresScalarEpilogue()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9167
     // ReductionOperations are orders top-down from the phi's use to the
-    // LoopExitValue. We keep a track of the previous item (the Chain) to tell
+    // LoopLiveOut. We keep a track of the previous item (the Chain) to tell
     // which of the two operands will remain scalar and which will be reduced.
----------------
ditto regarding retention of LoopExitValue vs. VPlanLiveOut.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:651
+void VPLiveOut::fixPhi(VPlan &Plan, VPTransformState &State) {
+  auto Lane = VPLane::getLastLaneForVF(State.VF);
+  if (Plan.isUniformAfterVectorization(getOperand(0)))
----------------
nits: set ExitingVPValue to operand(0), used twice.
Derive and set MiddleVPBock from loop/outermost region enclosing ExitingVPValue, rather than from Plan? Or OTOH have it passed in, as it’s the same for all phis to fix.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1103
+void VPlan::addLiveOut(PHINode *PN, VPValue *V) {
+  assert(LiveOuts.find(PN) == LiveOuts.end() &&
+         "an exit value for PN already exists");
----------------
assert count is zero?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:211
+    }
   return true;
 }
----------------
nit: empty line before final return true?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537