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

[Satish K Guggilla via Phabricator via llvm-commits]

sguggill added a comment.

The changes look good to me. I have also verified that my outer loop vector code generation changes work with these changes after some minor modifications.



================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:76
+      Instruction *Inst = dyn_cast<Instruction>(VPInst->getUnderlyingValue());
+      if (DeadInstructions.count(Inst) || isa<DbgInfoIntrinsic>(Inst)) {
+        Ingredient->eraseFromParent();
----------------
fhahn wrote:
> 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<>
OK.


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:94
+      } else
+        NewRecipe = new VPWidenRecipe(Inst);
+
----------------
dcaballe wrote:
> fhahn wrote:
> > 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.
> You mean in line 95? I see. We shouldn't create unnecessary recipes if we can reuse an existing one. Could we do something similar to the code at the end of 'tryToWiden'?:
> 
> ```
>   // Success: widen this instruction. We optimize the common case where
>   // consecutive instructions can be represented by a single recipe.
>   if (!VPBB->empty()) {
>     VPWidenRecipe *LastWidenRecipe = dyn_cast<VPWidenRecipe>(&VPBB->back());
>     if (LastWidenRecipe && LastWidenRecipe->appendInstruction(I))
>       return true;
>   }
> 
>   VPBB->appendRecipe(new VPWidenRecipe(I));
> ```
This looks good. Thanks!


https://reviews.llvm.org/D46827