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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

In https://reviews.llvm.org/D46827#1123771, @fhahn wrote:

> Updated to not use VPRecipeBuilder for recipe construction. It now relies relies on the original VPlan to be vectorizable without masking/interleaving. If that makes sense, I'll move the code enabling the CG to a separate patch and add a unit test for the transformation.


Thanks, Florian! Much better now! Some comments below.

> If that makes sense, I'll move the code enabling the CG to a separate patch and add a unit test for the transformation.

Please go ahead!

Let's see how this patch aligns with Satish's patch when it's ready.

Thanks,
Diego.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6395
+  if (EnableVPlanNativePath) {
+    VPlanHCFGTransforms::sinkInstructions(VPlans.front(),
+                                          Legal->getSinkAfter());
----------------
I think this might not be needed at this point?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6398
+
+    VPRecipeBuilder RecipeBuilder(OrigLoop, TLI, TTI, Legal, CM, Builder);
+    SmallPtrSet<Instruction *, 4> DeadInstructions;
----------------
unused?


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:60
+
+VPlanPtr VPlanHCFGTransforms::VPInstructionsToVPRecipies(
+    Loop *OrigLoop, VPlanPtr &OriginalPlan,
----------------
Since this transformation happens just before the execution of VPlan, I think it would be better to just modify the existing VPlan (i.e., remove each VPInstruction and add the corresponding recipes) instead of creating a new one. In that way, this code would be even simpler. Does it make sense to you or did you decide to create a new one for some reason?


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:74
+
+  VPRegionBlock *TopRegion = dyn_cast<VPRegionBlock>(OriginalPlan->getEntry());
+  ReversePostOrderTraversal<VPBlockBase *> RPOT(TopRegion->getEntry());
----------------
We should always have a TopRegion. dyn_cast -> cast?


================
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()))
----------------
Please, remove TopRegion check.


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp:95
+      Instruction *Inst = dyn_cast<Instruction>(VPInst->getUnderlyingValue());
+      if (DeadInstructions.count(Inst) || isa<DbgInfoIntrinsic>(Inst))
+        continue;
----------------
Shoudn't we add DbgInfoIntrinsic to DeadInstructions then?


https://reviews.llvm.org/D46827