[PATCH] D154240: [LV] Update generateInstruction to return produced value (NFC).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:850-851
   /// Utility method serving execute(): generates a single instance of the
-  /// modeled instruction.
-  void generateInstruction(VPTransformState &State, unsigned Part);
+  /// modeled instruction. \returns the generated value for \p Part or nullptr
+  /// if the generate instruction doesn't produce a value.
+  Value *generateInstruction(VPTransformState &State, unsigned Part);
----------------
Ayal wrote:
> This method should arguably always generate an Instruction and return it. What to do with the generated Instruction, when this recipe !hasResult, could be left to the caller.
> Cases that currently do not generate any instruction should be optimized away by (a) having the caller recognize that a single value is needed per all parts, and (b) hooking up existing values directly to their users w/o going through a VPInstruction. These can be dealt with separately, by subsequent patches.
updated thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:291
     Value *Next = nullptr;
     if (Part == 0) {
       bool IsNUW = getOpcode() == VPInstruction::CanonicalIVIncrementNUW;
----------------
Ayal wrote:
> Once the caller recognizes this as a uniform recipe, it should call it with Part zero only, and this "if" should be replaced by an assert.
Will do thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:308
+    if (Part == 0)
+      return IV;
 
----------------
Ayal wrote:
> Another case where an existing Value is returned instead of a newly generated instruction.
> (Optimizing away a redundant add-with-zero for Part zero by not creating it rather than Builder folding.)
Right, builder fold should be added in D152660.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:317
     if (Part != 0)
-      break;
+      return nullptr;
 
----------------
Ayal wrote:
> Once the caller recognizes this as a uniform recipe, it should call it with Part zero only, and this "if" should be replaced by an assert.
Will do, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:334
     Builder.GetInsertBlock()->getTerminator()->eraseFromParent();
-    break;
+    return nullptr;
   }
----------------
Ayal wrote:
> Perhaps better return CondBr, i.e., have generateInstruction() always return the instruction it generates (or an existing Value) and have the caller reason about setting it in State if hasResult/users or not?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:338
     if (Part != 0)
-      break;
+      return nullptr;
     // First create the compare.
----------------
Ayal wrote:
> Once the caller recognizes this as a uniform recipe, it should call it with Part zero only, and this "if" should be replaced by an assert.
Will do, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:358
     Builder.GetInsertBlock()->getTerminator()->eraseFromParent();
-    break;
+    return nullptr;
   }
----------------
Ayal wrote:
> Better return CondBr?
updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:371
+    Value *V = generateInstruction(State, Part);
+    if (!V)
+      continue;
----------------
Ayal wrote:
> Check instead if (!hasResult() - also considers e.g. stores) or !have any user?
> This is only to save compile-time - should be ok to set the State in any case, also if V is a branch (or null) - the State should not be get'd later if this VPInstruction has no result or users.
Updated to check `hasResult`, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154240