This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Add VPUserID to distinguish between recipes and others.
ClosedPublic

Authored by fhahn on Apr 11 2021, 4:11 AM.

Details

Summary

This allows cast/dyn_cast'ing from VPUser to recipes. This is needed
because there are VPUsers that are not recipes.

Diff Detail

Event Timeline

fhahn created this revision.Apr 11 2021, 4:11 AM
fhahn requested review of this revision.Apr 11 2021, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 4:11 AM
Herald added a subscriber: vkmr. · View Herald Transcript
a.elovikov added inline comments.Apr 12 2021, 11:40 AM
llvm/lib/Transforms/Vectorize/VPlan.h
686

Can we make getVPUserID() protected instead of friendship above?

llvm/lib/Transforms/Vectorize/VPlanValue.h
281
  1. Why is that important? Do we rely on it or is it simply to prevent uncontrolled growth?
  2. Why do we expect it to be the same on 32/64-bit platforms? Don't we have any pointers inside the object? I'd expect the vtable itself would make it different...
fhahn updated this revision to Diff 338880.Apr 20 2021, 8:24 AM

Add comment about the Other ID type, making clear that currently there are VPUsers in VPBlockBase, but in the future Other should only be used for live-outs.

fhahn added inline comments.Apr 20 2021, 8:27 AM
llvm/lib/Transforms/Vectorize/VPlan.h
686

Unfortunately I don't think so. If it's protected, we can access it from inside a VPRecipeBase object, but here we need to access it from a VPUser pointer directly.

llvm/lib/Transforms/Vectorize/VPlanValue.h
281

Why is that important? Do we rely on it or is it simply to prevent uncontrolled growth?

It's just to prevent accidental growth, but it is not strictly necessary (and we don't have static asserts for other sizes yet), but it is a common pattern, e.g. in various key classes in Clang to prevent unnoticed size growth.

Why do we expect it to be the same on 32/64-bit platforms?

I think 48 should be the upper bound on 64 bit platforms (unless there are differences caused by different 64 bit operating systems) and on 32-bit platforms it should be less than or equal.

a.elovikov accepted this revision.Apr 26 2021, 9:29 AM

LGTM.

llvm/lib/Transforms/Vectorize/VPlan.h
686

I'd rather make the enum/method public then, but it's very subjective.

This revision is now accepted and ready to land.Apr 26 2021, 9:29 AM
Ayal added a comment.Apr 26 2021, 10:47 PM

Non-recipe "other" VPUsers are quite exceptional ... would it suffice to have (native) VPlan hold all 'dangling' non-recipe VPUsers of its VPBasicBlocks, as an alternative, until these are cleaned up?

fhahn added a comment.Apr 29 2021, 1:56 AM

Non-recipe "other" VPUsers are quite exceptional ... would it suffice to have (native) VPlan hold all 'dangling' non-recipe VPUsers of its VPBasicBlocks, as an alternative, until these are cleaned up?

The blocks need to use the VPUsers to access the current value they hold I think, so I am not sure how the VPlan holding the VPUsers would look like unfortunately. It would be great if you could elaborate in a bit more detail, in case I am missing something.

Ayal added a comment.Apr 29 2021, 2:07 AM

Non-recipe "other" VPUsers are quite exceptional ... would it suffice to have (native) VPlan hold all 'dangling' non-recipe VPUsers of its VPBasicBlocks, as an alternative, until these are cleaned up?

The blocks need to use the VPUsers to access the current value they hold I think, so I am not sure how the VPlan holding the VPUsers would look like unfortunately. It would be great if you could elaborate in a bit more detail, in case I am missing something.

The blocks will still have their VPUsers (in native; until this gets cleaned up). The thought is that whoever needs to distinguish between a block'd VPUser and a recipe'd one, would do so by consulting VPlan, rather than isa-ing the VPUser.

fhahn added a comment.Apr 29 2021, 1:06 PM

Non-recipe "other" VPUsers are quite exceptional ... would it suffice to have (native) VPlan hold all 'dangling' non-recipe VPUsers of its VPBasicBlocks, as an alternative, until these are cleaned up?

The blocks need to use the VPUsers to access the current value they hold I think, so I am not sure how the VPlan holding the VPUsers would look like unfortunately. It would be great if you could elaborate in a bit more detail, in case I am missing something.

The blocks will still have their VPUsers (in native; until this gets cleaned up). The thought is that whoever needs to distinguish between a block'd VPUser and a recipe'd one, would do so by consulting VPlan, rather than isa-ing the VPUser.

Oh I see what you mean now, thanks! That sounds like a viable alternative.

IIUC we will have non-recipe users in the future (e.g. live-outs) so I think we need to be able to make this distinction in the future as well, once the current VPUsers in the native path get cleaned up. If that's true, I think having to consult a VPlan to check what type of user we are dealing with seems a bit clunky in the long term (and breaks with the common pattern used for other VP* classes and other places in LLVM). Unless we anticipate a more fine-grained distinction between VPUsers in the future, we could also just use a boolean flag instead of the enum, to make it less appealing to add new VPUser types.

If we don't anticipate having to distinguish between recipe and non-recipe VPUsers in the future, then I think using a map in the plan directly makes a lot of sense (and in that case we might want to get rid of the separate VPUser class completely?)

Kazhuu added a subscriber: Kazhuu.Apr 29 2021, 11:57 PM

IIUC we will have non-recipe users in the future (e.g. live-outs)

Not making a request or committing to implement myself in near future, but can we model live-outs as a recipe as well? I'm thinking about something like vplan-ret %val0, %val1, %val2 that would have VPValues as operands and corresponding llvm::Uses inside itself (like shufflevector contains mask argument or `PHINode' contains incoming blocks.

gilr added a comment.May 3 2021, 2:48 AM

Non-recipe "other" VPUsers are quite exceptional ... would it suffice to have (native) VPlan hold all 'dangling' non-recipe VPUsers of its VPBasicBlocks, as an alternative, until these are cleaned up?

The blocks need to use the VPUsers to access the current value they hold I think, so I am not sure how the VPlan holding the VPUsers would look like unfortunately. It would be great if you could elaborate in a bit more detail, in case I am missing something.

The blocks will still have their VPUsers (in native; until this gets cleaned up). The thought is that whoever needs to distinguish between a block'd VPUser and a recipe'd one, would do so by consulting VPlan, rather than isa-ing the VPUser.

Oh I see what you mean now, thanks! That sounds like a viable alternative.

IIUC we will have non-recipe users in the future (e.g. live-outs) so I think we need to be able to make this distinction in the future as well, once the current VPUsers in the native path get cleaned up. If that's true, I think having to consult a VPlan to check what type of user we are dealing with seems a bit clunky in the long term (and breaks with the common pattern used for other VP* classes and other places in LLVM). Unless we anticipate a more fine-grained distinction between VPUsers in the future, we could also just use a boolean flag instead of the enum, to make it less appealing to add new VPUser types.

If we don't anticipate having to distinguish between recipe and non-recipe VPUsers in the future, then I think using a map in the plan directly makes a lot of sense (and in that case we might want to get rid of the separate VPUser class completely?)

The anticipated long-term need for non-recipe VPUsers is indeed to represent live-outs, along with their underlying IR Value, or rather Instruction; analogous to VPValue::getLiveInIRValue(). This would call for a derived VPLiveOut non-recipe subclass of VPUser.

VPUser class itself should probably be pure virtual, similar to User, to avoid having a potential kitchen sink of dangling opaque instances. Instead of representing arbitrary "other"/"non-recipe" instances, the current VPUsers of VPBasicBlock predicates and conditions could be represented as specific, concrete subclasses of VPUser, potentially until they are cleaned up. WDYT?

llvm/lib/Transforms/Vectorize/VPlanValue.h
217–218

Do we have to have a default constructor? If so, shouldn't the default ID be Other?

Ayal added a comment.May 3 2021, 2:52 AM

The anticipated long-term need for non-recipe VPUsers is indeed to represent live-outs, along with their underlying IR Value, or rather Instruction; analogous to VPValue::getLiveInIRValue(). This would call for a derived VPLiveOut non-recipe subclass of VPUser.

VPUser class itself should probably be pure virtual, similar to User, to avoid having a potential kitchen sink of dangling opaque instances. Instead of representing arbitrary "other"/"non-recipe" instances, the current VPUsers of VPBasicBlock predicates and conditions could be represented as specific, concrete subclasses of VPUser, potentially until they are cleaned up. WDYT?

+1

llvm/lib/Transforms/Vectorize/VPlanValue.h
191

nit: above comment needs updating.

fhahn updated this revision to Diff 342375.May 3 2021, 6:45 AM

Rebased and addressed comments:

  1. Turn VPUser into a pure virtual class.
  2. Add VPBlockUser, move update logic to that class
  3. Remove default values for ID in constructors.
  4. Remove default constructor.
fhahn added a comment.May 3 2021, 6:47 AM

The anticipated long-term need for non-recipe VPUsers is indeed to represent live-outs, along with their underlying IR Value, or rather Instruction; analogous to VPValue::getLiveInIRValue(). This would call for a derived VPLiveOut non-recipe subclass of VPUser.

VPUser class itself should probably be pure virtual, similar to User, to avoid having a potential kitchen sink of dangling opaque instances. Instead of representing arbitrary "other"/"non-recipe" instances, the current VPUsers of VPBasicBlock predicates and conditions could be represented as specific, concrete subclasses of VPUser, potentially until they are cleaned up. WDYT?

+1

Thanks, I updated the code to turn VPUser in a virtual base class and added s subclass for the uses in VPBlockBase. WDYT?

(For now I used a dummy function VPUser::makeVPUserVirtual to turn VPUser into a pure virtual class. Not sure if there's a different way?)

fhahn added a comment.May 9 2021, 2:19 AM

@gilr @Ayal what are your thoughts on the latest changes? Would you prefer a different direction?

gilr added a comment.May 10 2021, 4:03 AM

Thanks, I updated the code to turn VPUser in a virtual base class and added s subclass for the uses in VPBlockBase. WDYT?

Looks good!

(For now I used a dummy function VPUser::makeVPUserVirtual to turn VPUser into a pure virtual class. Not sure if there's a different way?)

Wouldn't making the constructors protected suffice?

llvm/lib/Transforms/Vectorize/VPlan.h
686

I'd rather make the enum/method public then, but it's very subjective.

+1

gilr added inline comments.May 10 2021, 6:34 AM
llvm/lib/Transforms/Vectorize/VPlanValue.h
281

This seems a bit premature. Perhaps better to have this discussion separate of this patch.

fhahn updated this revision to Diff 344192.May 10 2021, 1:52 PM

Update to make getVPUserID() and the ID enum public, make constructors protected and remove static assert.

Thanks, I updated the code to turn VPUser in a virtual base class and added s subclass for the uses in VPBlockBase. WDYT?
(For now I used a dummy function VPUser::makeVPUserVirtual to turn VPUser into a pure virtual class. Not sure if there's a different way?)

Wouldn't making the constructors protected suffice?

Yeah, I guess we do not need it to be pure virtual, updated!

fhahn added inline comments.May 10 2021, 1:55 PM
llvm/lib/Transforms/Vectorize/VPlan.h
686

I don't mind either way, I made it public. But this has the dis-advantage that the IDs themselves are exposed to users, which should never use them. Probably not a very big deal though.

llvm/lib/Transforms/Vectorize/VPlanValue.h
281

It is just an extra guard which make unintended size changes visible (similar to other places in LLVM/Clang), but it can be a separate patch. I removed it for now.

fhahn updated this revision to Diff 344341.May 11 2021, 3:11 AM

Remove another unnecessary friendship relation.

fhahn added a comment.May 16 2021, 2:53 AM

@gilr I think your comments should be addressed and I am planning land this in the next few days, unless there are any additional comments :)

gilr accepted this revision.May 18 2021, 12:31 AM

@gilr I think your comments should be addressed and I am planning land this in the next few days, unless there are any additional comments :)

Yes they are, and no further comments :) ... LGTM!