[PATCH] D144125: [VPlan] VPWidenIntOrFpInductionRecipe inherits from VPHeaderPHIRecipe

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1142-1143
+  /// Returns the step value of the induction.
+  VPValue *getStepValue() { return getOperand(1); }
+  const VPValue *getStepValue() const { return getOperand(1); }
+
----------------
michaelmaitland wrote:
> Ayal wrote:
> > Now that `VPWidenIntOrFpInductionRecipe` inherits from `VPHeaderPHIRecipe`, it also inherits `getBackedgeValue()`, which should return a VPValue defined by some recipe - `getBackedgeRecipe()` - **inside** the loop. The default returns `getOperand(1)`. However, `VPWidenIntOrFpInductionRecipe` has no such VPValue nor recipe to return because it takes care of generating the corresponding IR code only during execution. Better override `getBackedgeValue()` and `getBackedgeRecipe()`, asserting they are unreachable/never called?
> > (`Step` which here serves as operand 1 instead is a value defined by a recipe in the loop preheader or constant - not a recipe.)
> I'm happy to add the overridden methods with unreachables in them for now. However, I think in the future it would be nice if a recipe extends another recipe, all operands of the base recipe must exist and be at the same index in the parent recipe. If not all derived versions of HeaderPHIRecipes have backedge values and recipes, then it should not belong in the base class. 
Agreed. This calls for a refactoring of VPWidenIntOrFpInductionRecipe, which will hopefully simplify its execute() to avoid inserting IR code in multiple points. Also calls for checking other derivatives of HeaderPHIRecipe, including VPWidenPointerInductionRecipe. Worth leaving behind a TODO to clean-up them unreachables?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144125