[PATCH] D147471: [VPlan] Don't add live-outs if scalar epilogue is required.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 5 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3723
   Loop *VectorLoop = LI->getLoopFor(State.CFG.VPBB2IRBB[LatchVPBB]);
-  if (Cost->requiresScalarEpilogue(VF)) {
-    // No edge from the middle block to the unique exit block has been inserted
-    // and there is nothing to fix from vector loop; phis should have incoming
-    // from scalar loop only.
-    Plan.clearLiveOuts();
-  } else {
+  if (!Cost->requiresScalarEpilogue(VF)) {
     // If we inserted an edge from the middle block to the unique exit block,
----------------
Ayal wrote:
> nit: retain explanation for the emptied else?
> 
> TODO: better understand from VPlan if any external (live-out?) users of IVs need to be fixed up, than to ask Cost?
Retained the comment and added the TODO. For fixed order recurrences, I already put up D147472 to do so


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8806
                                 VPBasicBlock *MiddleVPBB, Loop *OrigLoop,
-                                VPlan &Plan) {
+                                VPlan &Plan, bool RequiresScalarEpilogue) {
+  if (RequiresScalarEpilogue) {
----------------
Ayal wrote:
> Passing a parameter only to early-exit; better guard the call instead?
Removed arg, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8811
+    // from scalar loop only.
+
+    return;
----------------
Ayal wrote:
> nit: redundant empty line?
code is gone


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8997
+  addUsersInExitBlock(HeaderVPBB, MiddleVPBB, OrigLoop, *Plan,
+                      all_of(Range, RequiresScalarEpilogue));
 
----------------
Ayal wrote:
> How about delegating to CM the check if a Range requires a scalar epilog or not:
> ```
> bool RequiresScalarEpilogue = CM.requiresScalarEpilogue(Range);
> if (!RequiresScalarEpilogue) // No users to add when scalar epiloque must be reached. 
>   addUsersInExitBlock(HeaderVPBB, MiddleVPBB, OrigLoop, *Plan);
> ```
> ?
Moved the code to the cost model and also retained the comment for the case scalar epilogue is required.


================
Comment at: llvm/test/Transforms/LoopVectorize/loop-form.ll:201
 ; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr i32, ptr [[PTR:%.*]], i64 [[TMP0]]
 ; CHECK-NEXT:    [[TMP2:%.*]] = getelementptr i32, ptr [[TMP1]], i32 0
 ; CHECK-NEXT:    store <2 x i32> <i32 10, i32 10>, ptr [[TMP2]], align 4
----------------
Ayal wrote:
> Load eliminated from vectorized loop body (only), because we now recognize it is useless?
Yes, load is not needed as it is not used inside the vector loop and the scalar epilogue will take care of the value used outside the loop. Not adding VPLiveOuts allows the load to be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147471