[PATCH] D100257: [VPlan] Add VPUserID to distinguish between recipes and others.

[Gil Rapaport via Phabricator via llvm-commits]

gilr added a comment.

In D100257#2726688 <https://reviews.llvm.org/D100257#2726688>, @fhahn wrote:

> In D100257#2725036 <https://reviews.llvm.org/D100257#2725036>, @Ayal wrote:
>
>> In D100257#2725020 <https://reviews.llvm.org/D100257#2725020>, @fhahn wrote:
>>
>>> In D100257#2718728 <https://reviews.llvm.org/D100257#2718728>, @Ayal wrote:
>>>
>>>> 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?



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:212
 public:
-  VPUser() {}
-  VPUser(ArrayRef<VPValue *> Operands) {
+  VPUser() : ID(VPUserID::Recipe) {}
+  VPUser(ArrayRef<VPValue *> Operands, VPUserID ID = VPUserID::Other) : ID(ID) {
----------------
Do we have to have a default constructor? If so, shouldn't the default ID be Other?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100257