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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for following-up on this!
Adding some comments to hopefully further simplify and clarify the roles of generateInstruction() vs. execute().



================
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);
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:275
+    if (!PartMinus1->getType()->isVectorTy())
+      return PartMinus1;
+    Value *V2 = State.get(getOperand(1), Part);
----------------
Note that in this case an existing Value is returned rather than a newly generated Instruction.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:291
     Value *Next = nullptr;
     if (Part == 0) {
       bool IsNUW = getOpcode() == VPInstruction::CanonicalIVIncrementNUW;
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:308
+    if (Part == 0)
+      return IV;
 
----------------
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.)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:317
     if (Part != 0)
-      break;
+      return nullptr;
 
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:334
     Builder.GetInsertBlock()->getTerminator()->eraseFromParent();
-    break;
+    return nullptr;
   }
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:338
     if (Part != 0)
-      break;
+      return nullptr;
     // First create the compare.
----------------
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.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:371
+    Value *V = generateInstruction(State, Part);
+    if (!V)
+      continue;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154240