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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8443
+  auto *PHIRecipe =
+      Instr->getType()->isVoidTy() ? nullptr : new VPPredInstPHIRecipe(Op);
   if (PHIRecipe) {
----------------
Above use of `Op` is unrelated to the fix, right?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8446
     Plan->removeVPValueFor(Instr);
     Plan->addVPValue(Instr, PHIRecipe);
+    setRecipe(Instr, PHIRecipe);
----------------
There seems to be some redundancy left here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8449
+    Plan->removeVPValueFor(Instr);
+    Plan->addVPValue(Instr, PHIRecipe);
+  } else
----------------
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?




================
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
----------------
Should VPlan's def/use verification checks and/or splice/FOR/header-phi recipes assert that values across the backedge are indeed such?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129436