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

[Satish K Guggilla via Phabricator via llvm-commits]

sguggill added inline comments.


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


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:94
+      } else
+        NewRecipe = new VPWidenRecipe(Inst);
+
----------------
I think we are diverging from the nonVPlanNativePath case, where a widenRecipe can have > 1 ingredients but this should be fine.


================
Comment at: unittests/Transforms/Vectorize/VPlanHCFGTest.cpp:143
+  SmallPtrSet<Instruction *, 1> DeadInstructions;
+  VPlanHCFGTransforms::VPInstructionsToVPRecipies(Plan, &Inductions,
+                                                  DeadInstructions);
----------------
VPInstructionsToVPRecipies -> VPInstructionsToVPRecipes

I noticed this while trying to test my outerloop vector code generation changes along with your latest changes.


https://reviews.llvm.org/D46827