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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

Thanks, updated the patch to combine widened instructions in single recipe, if last recipe was a VPWidenRecipe.



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


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:94
+      } else
+        NewRecipe = new VPWidenRecipe(Inst);
+
----------------
sguggill wrote:
> I think we are diverging from the nonVPlanNativePath case, where a widenRecipe can have > 1 ingredients but this should be fine.
Yep, this code creates a new WidenRecipe for each widened instruction because it is slightly more work to get the previous recipe, because we remove the current instruction. I can change it though if you think that's better.


================
Comment at: unittests/Transforms/Vectorize/VPlanHCFGTest.cpp:143
+  SmallPtrSet<Instruction *, 1> DeadInstructions;
+  VPlanHCFGTransforms::VPInstructionsToVPRecipies(Plan, &Inductions,
+                                                  DeadInstructions);
----------------
sguggill wrote:
> VPInstructionsToVPRecipies -> VPInstructionsToVPRecipes
> 
> I noticed this while trying to test my outerloop vector code generation changes along with your latest changes.
Ah yes, sorry about that. This instance slipped through when I fixed the typo.


https://reviews.llvm.org/D46827