[PATCH] D129436: [LV] Use PHI recipe instead of PredRecipe for subsequent uses.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8443
+  auto *PHIRecipe =
+      Instr->getType()->isVoidTy() ? nullptr : new VPPredInstPHIRecipe(Op);
   if (PHIRecipe) {
----------------
Ayal wrote:
> Above use of `Op` is unrelated to the fix, right?
This is actually an unnecessary change in the latest version of the patch, removed! But it turns out that the call to `getOrAddVPValue` is actually unnecessary and we can use `PredRecipe` instead: 6f7347b888a4


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8446
     Plan->removeVPValueFor(Instr);
     Plan->addVPValue(Instr, PHIRecipe);
+    setRecipe(Instr, PHIRecipe);
----------------
Ayal wrote:
> There seems to be some redundancy left here?
Yes, cleaned up! Only `setRecipe` remains.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8449
+    Plan->removeVPValueFor(Instr);
+    Plan->addVPValue(Instr, PHIRecipe);
+  } else
----------------
Ayal wrote:
> Re-associating Instr with PHIRecipe instead of some previous VPValue (i.e., PredRecipe) via removeVPValueFor()/addVPValue() in order to affect subsequent get[OrAdd]VPValue()'s, seems aligned with re-associating setRecipe() to affect subsequent getRecipe()'s. Should we retain the initial setRecipe() which aligns with initial addVPValue(), and introduce the ability to resetRecipe() here? Can also replace removeVPValue()(+addVPValue()) with replaceVPValue(). The other (two) instances of removeVPValue()/addVPValue() pairs, namely adjustRecipesForReductions() and when transforming InterleaveGroups, should not resetRecipe(), right?
> 
> 
Adding a `resetRecipe` would be an option, but resetting a VPValue/recipe should only be an exception. In this case it seems cleaner to just delay setting it. WDYT?


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-sink-replicate-region.ll:474
 ; CHECK-NEXT:   pred.load.succ:
-; CHECK-NEXT:     EMIT vp<[[SPLICE:%.+]]> = first-order splice ir<%.pn> ir<%l>
 ; CHECK-NEXT:   Successor(s): pred.sdiv
----------------
Ayal wrote:
> Should VPlan's def/use verification checks and/or splice/FOR/header-phi recipes assert that values across the backedge are indeed such?
I think a check that defs dominate uses would catch that in general without the need for special handling. It might be possible to extend the verifier without fully fledged VP dominator support.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129436/new/

https://reviews.llvm.org/D129436