Page MenuHomePhabricator

[VPlan] Make VPRecipeBase inherit from VPDef.
AcceptedPublic

Authored by fhahn on Nov 1 2020, 10:47 AM.

Details

Summary

This patch makes VPRecipeBase a direct subclass of VPDef, moving the
SubclassID to VPDef.

Diff Detail

Event Timeline

fhahn created this revision.Nov 1 2020, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2020, 10:47 AM
fhahn requested review of this revision.Nov 1 2020, 10:47 AM
fhahn updated this revision to Diff 302295.Mon, Nov 2, 8:42 AM

Small changes to fix remaining test failures.

fhahn updated this revision to Diff 302339.Mon, Nov 2, 10:51 AM

Remove toVPUser, which is unnecessary now

fhahn updated this revision to Diff 303239.Thu, Nov 5, 1:03 PM

Updated to keep converted recipes as VPValues.

fhahn updated this revision to Diff 303454.Fri, Nov 6, 7:32 AM

Update after VPDef does not inherit from VPUser directly.

fhahn updated this revision to Diff 303515.Fri, Nov 6, 11:53 AM

Another round of re-basing

gilr added inline comments.Sun, Nov 22, 5:33 AM
llvm/lib/Transforms/Vectorize/VPlan.cpp
120

IIUC this is still necessary since VPInstruction doesn't register itself as a VPValue in its VPDef (ownership issue?). Does this mean that using the VPDef::defined_values() API won't work when VPInstructions are involved?

fhahn added inline comments.Sun, Nov 22, 6:19 AM
llvm/lib/Transforms/Vectorize/VPlan.cpp
120

IIUC this is still necessary since VPInstruction doesn't register itself as a VPValue in its VPDef (ownership issue?). Does this mean that using the VPDef::defined_values() API won't work when VPInstructions are involved?

Using VPDef for VPInstruction required a bit of extra churn in the original version of the patches. That is why the patch turning VPInstruction into a VPDef is a bit out-of-order. D90565 turns VPInstruction into a VPDef and allows us to get rid of toVPValue. I just updated the patch to do so. With that patch, definedValues should work as expected for all recipes.

gilr requested changes to this revision.Tue, Nov 24, 6:09 AM
gilr added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
607

Should there be a VPRecipeBase::classof(VPDef *) to support dyn_cast from VPDef to VPRecipeBase?

688–698

Recipe -> Def

705

Worth documenting here too the importance of the inheritance order.

This revision now requires changes to proceed.Tue, Nov 24, 6:09 AM
fhahn updated this revision to Diff 307362.Tue, Nov 24, 7:53 AM

Address comments

fhahn marked an inline comment as done.Tue, Nov 24, 7:55 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
607

yes I added one. For now, it just returns true, as all VPDefs are also VPRecipeBases at the moment. If they diverge, we need to check the IDs.

688–698

I also updated the other classof calls which were using VPDef *V.

705

I added a comment to VPRecipeBase. Otherwise we would have to add the same comment to all subclasses of VPRecipeBase. Do you think that is sufficient or should I add the comment to the subclasses?

gilr accepted this revision.Tue, Nov 24, 9:08 AM

LGTM, tx!

This revision is now accepted and ready to land.Tue, Nov 24, 9:08 AM
fhahn marked an inline comment as done.

LGTM, tx!

Great thanks! This depends on a set of patches turning individual recipes into VPDefs. They should be linked correctly in Phabricator now.