[PATCH] D84684: [VPlan] Use VPValue def for VPInterleaveRecipe.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D84684#2314359 <https://reviews.llvm.org/D84684#2314359>, @dmgreen wrote:

>> Agreed, we could also model it so that VPInterleaveRecipe creates a single vector with all loaded values and then use extract instructions for the different parts of the interleave group. This keeps the modeling in the VPValue classes & co a bit simpler. But it has the cost that we need to add additional instructions/recipes to the plan, which need to be considered by other analyses and transformations.  Another problem is that we would have to materialize a vector with all loaded values, which is completely artificial.
>
> I was considering that the VPInterleaveRecipe and the VPExtract's would work in tandem. You would never get the "all loaded values" vector, it would just create the interleaving gorup as it does at the moment, with the VPExtracts representing the values for each item. They would still be special values, VPExtracts would be the only type of recipe that could use a VPInterleave, and the VPExtract would not really contain anything in it's execute method.
>
> You could just consider the VPExtract as a new type of VPUser, not a VPRecipe directly. It's really a convenient way of using the operands to join up the two elements, not having to rely upon UnderlyingOrProducerTy. I guess that's a lot like the VPMultiValue concept. Using a new type of VPUser removes the complexity from VPValue at least!

IICU conceptually the suggested `VPExtract` and the 'sub-value' are effectively the same thing, just slightly different in terms where exactly they are defined.

I think most of the trade-offs are still similar, with a VPExtract version probably ending up slightly simpler and the sub-value one being a bit more complex but also a bit more explicit in the modeling and more flexible in the future. I think as-is, the 'sub-value' approach is relatively lightweight and does not add too much complexity and the difference between the VPExtract alternative should be relatively small. But if people generally prefer this approach I am happy to update the patches.

>> I think we have the make the following trade-off. Either we add additional complexity to VPValue & co to model recipes that produce multiple VPValues or we add additional instructions/recipes to the plans we generate.
>>
>> Personally I think the additional complexity in VPValue & co is worth having simpler plans. Also, there might be additional complex recipes that produce multiple values in the future, which would also benefit from dedicated multi-value support.
>
> It feels like we have one type system (VPValueSC/VPInstructionSC/VPWidenSC/etc), and we are adding another system on top of it, for different types of VPValue via UnderlyingOrProducerTy being virtual or subvalues or concrete.

That definitely can be improved. We should probably just rely on the VPValueID and use that to extract the right value from a union instead of sum type.

> Is there a nice way at the moment of detecting _which_ output you are seeing from a subvalue VPValue? As in which member index it would be from a interleaved group.

Currently there's no nice way, but it can easily be done by getting the defining value and then checking which def corresponds to the sub-value. It should be possible to add a convenient interface once there's a need for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84684