[PATCH] D46827: [VPlan] Add VPInstruction to VPRecipe transformation.

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

Thanks for the rework and for your patience, Florian!
I don't see any major issues. Please, wait for Satish's approval, just in case he has any comments regarding how this would interact with his next patch.
In any case, we could always address any issues in a separate commit.

Diego



================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:80
+    // care of creating instructions in entry and exit blocks.
+    if (TopRegion && (OriginalVPBB == TopRegion->getEntry() ||
+                      OriginalVPBB == TopRegion->getExit()))
----------------
fhahn wrote:
> dcaballe wrote:
> > Please, remove TopRegion check.
> Done. I think we still need a check to not create recipes for the pre-header and exit blocks?
Yes, you can do that for now. I plan to create clean/empty PH and Exit during construction so that we can safely vectorize or process whatever we add to them other VPlan-to-VPlan transformations.


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:76
+      Instruction *Inst = dyn_cast<Instruction>(VPInst->getUnderlyingValue());
+      if (DeadInstructions.count(Inst) || isa<DbgInfoIntrinsic>(Inst)) {
+        Ingredient->eraseFromParent();
----------------
Shoudn't we collect DbgInfoIntrinsic as a DeadInstructions instead of having this check here?


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:83
+      // Create VPWidenMemoryInstructionRecipe for loads and stores.
+      if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst)) {
+        NewRecipe = new VPWidenMemoryInstructionRecipe(*Inst, nullptr /*Mask*/);
----------------
remove { } for single line ifs and elses?


================
Comment at: unittests/Transforms/Vectorize/VPlanHCFGTest.cpp:38
+
+TEST_F(VPlanHCFGTest, testBuildHCFGInnerLoop) {
+  LLVMContext Ctx;
----------------
Thanks a lot for these tests!


https://reviews.llvm.org/D46827