[PATCH] D125301: [LoopVectorize] Add option to use active lane mask for loop control flow

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8712
+
+    auto *ALM = new VPInstruction(VPInstruction::ActiveLaneMask,
+                                  {CanonicalIVIncrementParts, TC}, DL);
----------------
It looks like the update for the phi doesn't depend on the phi, but only on trip count & main induction. 

I might be missing something, but is the phi actually needed or would it be possible to compute the active lane mask at the beginning of each iteration instead of at the end of the iteration?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1568
+        createStepForVF(State.Builder, TC->getType(), State.VF, Part);
+    Instruction *StartMask = State.Builder.CreateIntrinsic(
+        Intrinsic::get_active_lane_mask, {PredTy, TC->getType()}, {StartIV, TC},
----------------
The start value should be created in the preheader block in VPlan instead of in the phi node here. There there should also be no need for a TC argument?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:764
+    BranchOnCond,
+    BranchOnActiveLaneMask,
   };
----------------
Can `BranchOnCond` be used instead of the dedicated `BranchOnactiveLaneMask`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1916
+  DebugLoc DL;
+  VPValue *TC;
+
----------------
TC would need to be an operand instead of just a member variable, otherwise things will break if something does `TC->replaceAllUsesWith()` outside. 


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

https://reviews.llvm.org/D125301