[PATCH] D144125: [VPlan] Improve VPRecipeBase::isPhi checking

[Michael Maitland via Phabricator via llvm-commits]

michaelmaitland updated this revision to Diff 499265.
michaelmaitland added a comment.

In D144125#4142196 <https://reviews.llvm.org/D144125#4142196>, @fhahn wrote:

> Thanks for the patch.
>
>>> VPWidenIntOrFPInductionRecipe are not in the phi section of the header block for truncates of induction variables. Those recipes are moved to the phi section of the header block after applying SinkAfter, which relies on the original position of the trunc.
>
> I am not sure if this is still true, we should move them to the header at construction time. If so, It would be better to VPWidenIntOrFpInductionRecipe inherit from VPHeaderPHIRecipe, unless that causes any other issues. They have the same base classes, so that should be fine.

I just saw that D142589 <https://reviews.llvm.org/D142589> landed which removes the fact that `VPWidenIntOrFpInductionRecipe` are not in the header block for truncates of induction variables. So I think you are right that this is no longer a concern.

However, it is still the case the `VPHeaderPHIRecipe` only accepts `PHINode`s as its underlying value, but `VPWidenIntOrFpInductionRecipe` uses either `PHINode` or `TruncInst` as an underlying value. I see two approaches forward:

1. Modify `VPHeaderPHIRecipe` to accept any `Instruction` as its underlying value.
2. Only use `PHINode` as underlying value for `VPWidenIntOrFpInductionRecipe`.

I prefer option 1, as the only real change required is for the underlying instruction of a `VPHeaderPHIRecipe` to change from `PHINode -> Instruction`. Option 2 is more involved since we're currently creating two `VPWidenIntOrFpInductionRecipe` (one for the phi and one for the trunc), but we would need to only create one (since one recipe per underlying IR is permitted) and update the single recipe with the trunc information when we process the trunc as well as replace uses of the trunc with this recipe.

But I think also boils down to what does it mean to be a "phi-like" recipe. Does it mean that the input IR is phi like? Does it mean that the output IR is phi like? Do both need to be phi like? I think if we are okay with it meaning the output IR is phi like, then option 1 is acceptable. If we are okay with this definition of phi-like, then we are able to have phi-like VPInstructions (which do not have any input IR) in the future.

I have updated this patch with approach 1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144125

Files:
  llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
  llvm/lib/Transforms/Vectorize/VPlan.cpp
  llvm/lib/Transforms/Vectorize/VPlan.h
  llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
  llvm/lib/Transforms/Vectorize/VPlanValue.h
  llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144125.499265.patch
Type: text/x-patch
Size: 15165 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230221/7d78c5f7/attachment.bin>