This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] VPWidenIntOrFpInductionRecipe inherits from VPHeaderPHIRecipe
ClosedPublic

Authored by michaelmaitland on Feb 15 2023, 10:45 AM.

Details

Summary

VPWidenIntOrFpInductionRecipe inherits from VPHeaderPHIRecipe since this recipe is phi like and only gets placed in vector header vp basic block.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2023, 10:45 AM
michaelmaitland requested review of this revision.Feb 15 2023, 10:45 AM

Since VPWidenIntOrFpInductionRecipe exists is phi like and in the
header, but isnt a VPCSAHeaderPHIRecipe, I fix the verification check.
I think a isHeaderPhi function where recipes can report if they are
header phi like might be nice in the future.

Fixing syntax error from last update

Remove the ! that I incorrectly placed in one of the checks

Add assert before casting.

@fhahn is there a reason why VPWidenIntOrFpInductionRecipe does not extend VPHeaderPHIRecipe? That might be a better solution than this one.

@fhahn is there a reason why VPWidenIntOrFpInductionRecipe does not extend VPHeaderPHIRecipe? That might be a better solution than this one.

To respond to my own question:

I imagine VPWidenIntOrFpInductionRecipe does not extend VPHeaderPHIRecipe because if its changed to derive VPHeaderPHIRecipe, the PHI is the only one that can define this VPValue, even though there are two constructors which would like to set different underlying values for this VPValue:

  1. IV defines this VPValue
  2. Trunc defines this VPValue

An additional reason why it does not extend is:
> 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.

Therefore, I prefer the proposed solution.

Instead of using multiple isa checks in the same guard (and those same checks being duplicated across multiple ifs), move into utils method. Also move isPhi out of VPRecipeBase to reduce class bloat.

Update missed isHeaderPhi check

michaelmaitland edited the summary of this revision. (Show Details)Feb 20 2023, 12:35 PM

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.

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 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 PHINodes 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.

Remove stacked commit from revision https://reviews.llvm.org/D144125

michaelmaitland edited the summary of this revision. (Show Details)Feb 21 2023, 12:35 PM
michaelmaitland retitled this revision from [VPlan] Improve VPRecipeBase::isPhi checking to [VPlan] VPWidenIntOrFpInductionRecipe inherits from VPHeaderPHIRecipe.

Thanks for the update! For the phi-like terminology, what matter is whether the recipe merges incoming values from different blocks.

After changing VPWidenIntOrFpInductionRecipe, are the other changes still needed?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
10470 ↗(On Diff #499266)

that should not be needed I think, cast asserts that the argument isa VPHeaderPHIRecipe.

fhahn added inline comments.Feb 24 2023, 10:02 AM
llvm/lib/Transforms/Vectorize/VPlan.h
1179

Could you make sure that the code movement preserves the original code? I didn't check all lines, but it looks like the original code had a newline before here.

michaelmaitland marked an inline comment as done.

Remove unncessary assert and fix removal of blank line.

michaelmaitland marked an inline comment as done.Feb 24 2023, 10:28 AM

After changing VPWidenIntOrFpInductionRecipe, are the other changes still needed?

I think this change reduces class bloat (since not all recipes are phi like, header phi like, or generate backedge). It also makes it so we only use RecipeTy in classof and uses isa to determine whether something isPhi.

I have some changes I plan to upstream in the near future that I think will give these util functions an even stronger justification for existing. If you don't think that the justifications i give are strong enough now, I am happy to remove them from this patch and include them in the future patch I mentioned.

Remove another unnecessary assert

fhahn added a comment.Mar 5 2023, 12:10 PM

After changing VPWidenIntOrFpInductionRecipe, are the other changes still needed?

I think this change reduces class bloat (since not all recipes are phi like, header phi like, or generate backedge). It also makes it so we only use RecipeTy in classof and uses isa to determine whether something isPhi.

I have some changes I plan to upstream in the near future that I think will give these util functions an even stronger justification for existing. If you don't think that the justifications i give are strong enough now, I am happy to remove them from this patch and include them in the future patch I mentioned.

Usually we try to keep unrelated changes as separate patches, if possible. It seems like fixing the inheritance can be done on its own and the isPhi changes can be done & discussed separately from that issue.

Remove Util functions

michaelmaitland edited the summary of this revision. (Show Details)Mar 6 2023, 8:11 AM
fhahn accepted this revision.Mar 7 2023, 6:00 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 7 2023, 6:00 AM
Ayal added a comment.Mar 7 2023, 1:38 PM

This change makes a lot of sense, except for getBackedgeValue() - see comment.

llvm/lib/Transforms/Vectorize/VPlan.h
1212–1213

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

1217–1222

nit: these should now be one liners.

michaelmaitland marked 2 inline comments as done.Mar 13 2023, 8:08 AM
michaelmaitland added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
1212–1213

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.

michaelmaitland marked an inline comment as done.

Fix getBackedgeRecipe and getBackedgeValue

Ayal accepted this revision.Mar 14 2023, 3:54 AM
Ayal added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
1212–1213

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?

Leave TODO to clean up unreachables.

This revision was landed with ongoing or failed builds.Mar 14 2023, 5:02 PM
This revision was automatically updated to reflect the committed changes.