[PATCH] D119661: [LV] Support chained phis as incoming values for first-order recurs.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8882
       // value from the backedge after all recipes have been created.
-      recordRecipeOf(cast<Instruction>(
-          Phi->getIncomingValueForBlock(OrigLoop->getLoopLatch())));
+      auto *Inc = cast<Instruction>(
+          Phi->getIncomingValueForBlock(OrigLoop->getLoopLatch()));
----------------
david-arm wrote:
> This change looks like it might potentially affect reductions too, since now we're potentially not recording the recipe for reductions. Should there be a reduction test for this too, or an assert that `Ingredient2Recipe[Inc]` is always non-null for reductions?
I don't think the change should impact reductions, as the code here only skips recording `Inc` if it is not already marked for recording. I added an assertion that if we already have a recipe recorded at this point, it must be a reduction or first-order recurrence phi.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8884
+          Phi->getIncomingValueForBlock(OrigLoop->getLoopLatch()));
+      if (Ingredient2Recipe[Inc] == nullptr)
+        recordRecipeOf(Inc);
----------------
david-arm wrote:
> nit: Should this just be `!Ingredient2Recipe[Inc]` as I thought LLVM convention wasn't to compare ==/!= with nullptr?
I think it would be best to use find() as not to insert new entries. Updated.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9268
       Builder.setInsertPoint(InsertBlock, std::next(PrevRecipe->getIterator()));
-
     auto *RecurSplice = cast<VPInstruction>(
----------------
david-arm wrote:
> nit: Whitespace change.
removed , thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119661