[PATCH] D143865: [VPlan] Add predicate to VPReplicateRecipe, expand region later.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8606
+    VPValue *BlockInMask = createBlockInMask(I->getParent(), Plan);
+    Recipe->addOperand(BlockInMask);
+  }
----------------
nit: worth recording as Mask to be returned by getMask(), initially set to null, rather than having the latter return the last operand blindly below?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8618
   assert(Instr->getParent() && "Predicated instruction not in any basic block");
+  auto *BlockInMask = PredRecipe->getOperand(PredRecipe->getNumOperands() - 1);
+  auto *RecipeWithoutMask = new VPReplicateRecipe(
----------------
nit: worth adding a comment that the masked replicate recipe is replaced by a non-masked one?


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/consecutive-ptr-uniforms.ll:95
 ; FORCE-NEXT:    br i1 [[TMP3]], label [[PRED_LOAD_IF:%.*]], label [[PRED_LOAD_CONTINUE:%.*]]
-; FORCE:       pred.load.if:
+; FORCE:       pred.store.if:
 ; FORCE-NEXT:    [[TMP0:%.*]] = add i32 [[INDEX]], 0
----------------
nit: name of label fixed? Worth fixing them PRED_LOAD* names as well for consistency?


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-sink-replicate-region.ll:369
 ; CHECK-NEXT:     REPLICATE ir<%rem.div> = sdiv ir<20>, ir<%rem>
-; CHECK-NEXT:     REPLICATE ir<%gep> = getelementptr ir<%ptr>, vp<[[STEPS]]>
+; CHECK-NEXT:     REPLICATE ir<%gep> = getelementptr ir<%ptr>
 ; CHECK-NEXT:     REPLICATE store ir<%rem.div>, ir<%gep>
----------------
Second operand `vp<[[STEPS]]>` dropped intentionally?


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-sink-replicate-region.ll:371
 ; CHECK-NEXT:     REPLICATE store ir<%rem.div>, ir<%gep>
-; CHECK-NEXT:     REPLICATE ir<%gep.2> = getelementptr ir<%dst.2>, vp<[[STEPS]]>
+; CHECK-NEXT:     REPLICATE ir<%gep.2> = getelementptr ir<%dst.2>
 ; CHECK-NEXT:     REPLICATE store ir<%rem.div>, ir<%gep.2>
----------------
ditto


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-inloop-cond.ll:411
+; CHECK-NEXT:    [[TMP37:%.*]] = load i32, ptr [[TMP36]], align 4
+; CHECK-NEXT:    [[TMP38:%.*]] = insertelement <4 x i32> [[TMP30]], i32 [[TMP37]], i64 3
 ; CHECK-NEXT:    br label [[PRED_LOAD_CONTINUE6]]
----------------
Two independent replicating regions with a reduce.add between them that were kept separate are now being merged? This patch seems conceptually an NFC, according to its current commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143865