This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Manage pairs of incoming (VPValue, VPBB) in VPWidenPHIRecipe.
ClosedPublic

Authored by fhahn on Feb 16 2021, 5:29 AM.

Details

Summary

This patch extends VPWidenPHIRecipe to manage pairs of incoming
(VPValue, VPBasicBlock) in the VPlan native path. This is made possible
because we now directly manage defined VPValues for recipes.

By keeping both the incoming value and block in the recipe directly,
code-generation in the VPlan native path becomes independent of the
predecessor ordering when fixing up non-induction phis, which currently
can cause crashes in the VPlan native path.

This fixes PR45958.

Diff Detail

Event Timeline

fhahn created this revision.Feb 16 2021, 5:29 AM
fhahn requested review of this revision.Feb 16 2021, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 5:29 AM
Herald added a subscriber: vkmr. · View Herald Transcript
fhahn updated this revision to Diff 323965.Feb 16 2021, 5:34 AM

Fix FileCheck arg in test.

Kazhuu added a subscriber: Kazhuu.Feb 16 2021, 10:57 AM
sguggill requested changes to this revision.Feb 21 2021, 11:01 AM

Thanks for the changes Florian. Some comments:

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4366–4367

Please delete this now obsolete comment.

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
37

nit - please delete or update comment for VPWidenPHIRecipe case.

55–60

Do we need to create a new recipe here? Can we leave the original ingredient as is?

76–83

Please delete.

This revision now requires changes to proceed.Feb 21 2021, 11:01 AM
fhahn updated this revision to Diff 325327.Feb 21 2021, 11:30 AM

Address Satish's comments, thanks!

fhahn marked an inline comment as done.Feb 21 2021, 11:32 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4366–4367

removed, thanks! I think the comment for Builder.SetInsertPoint() is not relevant any longer, so I removed that code as well.

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
55–60

We can re-use the VPWidenPHIRecipe, updated!

76–83

Done, we should not have any VPValues with phis (and there's the assert).

sguggill accepted this revision.Feb 21 2021, 3:20 PM

Thanks for the changes Florian, LGTM.

This revision is now accepted and ready to land.Feb 21 2021, 3:20 PM
This revision was landed with ongoing or failed builds.Feb 22 2021, 1:46 AM
This revision was automatically updated to reflect the committed changes.

This commit probably broke some Vectorize tests according to the build bots.
Could you please have a look?

fhahn added a comment.Feb 22 2021, 2:36 AM

This commit probably broke some Vectorize tests according to the build bots.
Could you please have a look?

thanks, should be fixed by c11fd0df6429