[PATCH] D139074: Vectorization Of Conditional Statements Using BOSCC

[Matt D. via Phabricator via llvm-commits]

Matt added inline comments.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:441
+  if (VF <= 4) {
+    // Create the intermedeate vector type cast if required
+    // i.e.  promote <4 x i1> to <4 x i8> type
----------------



================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:442
+    // Create the intermedeate vector type cast if required
+    // i.e.  promote <4 x i1> to <4 x i8> type
+    Type *VecTy = FixedVectorType::get(Builder.getIntNTy(8), VF);
----------------
(Extremely minor / formatting: stray space replaced with a comma after `i.e.`)


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:457
+      // 1. SubVector Extract
+      // %vec.extract = shufflevector <128 x i1> %Vector, <128 x i1> %undef, <32
+      // x i32> Mask
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:207
+static cl::opt<unsigned> BOSCCInstructionInBlockThreshold(
+    "boscc-instructions-in-threshold", cl::init(5), cl::Hidden,
+    cl::desc("The minimum instructions in a block required for boscc"));
----------------
I'm curious about the difference from AOCC here. Context: My understanding (from the EuroLLVM 2023) talk is that this has been already implemented in AOCC before. I'm wondering, were the cost model decisions / profitability analysis the same? I'm curious about it in the context of the (already resolved) ICE--is it possible that AOCC was able to avoid running into an ICE whereas this patch previously exposed a possibly missing precondition/assumption (could there be any more follow-on issues?)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8167
+// BB.BOSCC.GUARD : This serves the purpose of guarding with right condition
+// BB.BOSCC.VEC : This is the vector block corrosponds to BB
+// BB.BOSCC.VEC.CONTINUE: Auxilary block to facilitate control flow
----------------
(Minor typo; tense): s/corrosponds/corresponds/


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8168
+// BB.BOSCC.VEC : This is the vector block corrosponds to BB
+// BB.BOSCC.VEC.CONTINUE: Auxilary block to facilitate control flow
+// BB.BOSCC.JOIN: Required for PHI generation for the live out from BB
----------------
(Minor typo) s/Auxilary/Auxiliary/


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8203
+    return false;
+  // If the instruction has usage across basic block
+  for (Use &U : I->uses()) {
----------------
(I presume we care about usage across more than one basic block in general?)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9067
     // ingredients and fill a new VPBasicBlock.
+    // Init the BOSCC Planner and check the legal and profitablity
+    BOSCCBlockPlanner BOSCCPlanner(BB, VPBB, OrigLoop, DT, &TTI, Builder);
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:55
 
+extern cl::opt<bool> EnableBOSCCVectorization;
+
----------------
Is it intentional to have `EnableBOSCCVectorization` in the global namespace (outside of `namespace llvm`, cf. `EnableVPlanNativePath` a few lines above)?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2768
+
+  /// Mark VPlan indicating
+  void markPlanWithBOSCC() { HasBOSCCBlocks = true; }
----------------
This comment appears to be unfinished.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1077
+// It will have 2 incoming values, first representing value from
+// vector block, and the second in undefined value when vector block
+// is not executed.
----------------
(update comment alone to reflect already correctly using `PoisonValue` in the code)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1093
+//   %12 = phi <8 x i32> [ %SomeComputedValue, %if.then.boscc.vec.continue ],
+//                       [ undef, %if.then.boscc.guard ]
+void VPBOSCCLiveOutRecipe::execute(VPTransformState &State) {
----------------
(update comment alone to reflect already correctly using `PoisonValue` in the code)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1094
+//                       [ undef, %if.then.boscc.guard ]
+void VPBOSCCLiveOutRecipe::execute(VPTransformState &State) {
+  VPValue *ScalarLiveOut = getOperand(0);
----------------
I think this function could benefit from more comments explaining the logic--similarly to comments in `createVectorToScalarCast` in this patch (which are pretty good!)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:151
+    if (isa<VPBOSCCLiveOutRecipe>(*RecipeI)) {
+      RecipeI++;
+      continue;
----------------
(Coding style) Nit: Preincrement form `++RecipeI` is preferable (particularly for an iterator), https://llvm.org/docs/CodingStandards.html#prefer-preincrement



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139074