[PATCH] D88380: [VPlan] Extend VPValue to also model sub- & 'virtual' values.

Mon Nov 9 02:39:48 PST 2020

fhahn added a comment.

In D88380#2381997 <https://reviews.llvm.org/D88380#2381997>, @dmgreen wrote:

> Hello Florian. Sorry for the delay. I believe that the design of something is the most important part to get right - and the hardest part to change.  I've been trying to take a look at these and the other new patches but I'm not sure I understand yet, likely because I have not had to deal with the complexities it is trying to address. I think my head-canon is simpler than yours, perhaps too simple! It mostly just thinks of things the same as IR instructions, which I like because it's familiar and dependable.

Thanks for taking a look. I realize those are a lot of changes. I think the points below are spot on, I tried to expand on a few.

> As far as I understand, correct me where I'm wrong:
>
> - Most instruction produce a single value. These are simple enough to deal with, we use a VPValue.
> - Most instructions also consume values, for which we use VPUser.
> - The complexity comes from interleaving groups which might need to deal with multiple values.

Yes, interleave groups are the only example in the current code. But there very likely will be additional ones in the future. Another example that Ayal mentioned some time ago is modeling `sincos` for which various vector libraries like Accelerate (https://developer.apple.com/documentation/accelerate/vforce/3241297-sincos) or in Intel's SVML provide tuned implementations, which return separate vectors with the `sin` and `cos` results.

One benefit of modeling the fact that we can have recipes that define multiple values is that VPlan based analysis/transformations need and can account for this scenario in general. This would hopefully ensure the code is written in a way to safely handle future additions of 'multi-defs'. If we just special case the interleave case, this likely is not the case and introducing new multi-defs in the future will be more painful.

Also there's a trend towards more specialized vector instruction sets and I expect more specialized candidates for multi-defs to pop up there as well. Unfortunately some of those are not public, but I think modeling the multi-def case in general should ensure VPlan can be used for such specialized cases downstream without to much pain as well in the future.

> - Interleaving stores do not produce multiple values, but do have multiple operands (?)

yes, some recipes like `VPWidenMemoryInstructionRecpipe`, `VPWidenCallRecipe` or `VPInterleaveRecipe` may not actually define a VPValue, depending on the contents. That makes turning them into VPValues a little awkward, so those are probably likely candidates to break-down further in the future (see response further below as well)

> - Interleaving loads do need to produce multiple values, in some way.
>   - They can either use something like this built into the VPValue class (which I kept getting stuck on having two different type systems in VPValue).
>   - They can have the Def/MultiDef from D90558 <https://reviews.llvm.org/D90558> etal (which looks cleaner than the first version I was trying to look at last week, but still involves a lot of little nodes).
>   - They could hold a vector of "Members" that are VPUsers and join those together with operands. The complexity gets moved into the VPInterleaveRecipe class and nothing is needed in VPValue/etc

IIUC this is the same idea as the `VPDef` approach (D90558 <https://reviews.llvm.org/D90558>), where `VPDef` adds such a vector, but in a general fashion so we do not need to special case VPInterleaveRecipe. I think this vector has to hold VPValues in some form. I am not entirely sure how the VPUser fits into this vector, as I think the whole interleave group shares a single address as operand.

Note that D90558 <https://reviews.llvm.org/D90558> & co should be simpler than the originally proposed MultiDef, which was spread out over multiple classes. The latest patches essentially just add the `VPDef` class, which just contains a vector of 'defined' VPValues. In the end, all recipes should inherit from it and VPDef can be folded directly into `VPRecipeBase`. So it  should boil down to adding a single vector to `VPRecipeBase`.

It also requires a change to `VPValue` to allow getting the `VPDef` (effectively the recipe) that defines the value. I think there's no way around that, even if we just change VPInterleaveRecipe to keep a vector of VPValues it defines. To walk upwards the def-use chains, we need to be able to get the recipe/VPDef that defines a VPValue. In the single def cases, we can just cast the VPValue directly to the corresponding recipe, but that's not possible for VPValues defined by a 'multi-def'. Once we have to do that, I think we might as well add a general way to express that a recipe can define multiple values.

> As you might have guessed, I still quite like the last option. But that might well just be because there are somethings about it that I haven't had to deal with. I like how it mirrors the llvm-ir though, it to me seems simple and familiar.
>
> Some other random questions:
>
> - Do you know how Store Interleaving recipes should be handled?

(See below)

> - Do you think that vplan nodes will eventually need types? (From looking at some things I think the answer is probably yes).

Once we synthesize more VPInstructions or recipes that are not directly tied to the original IR, I think we will need a way to attach types to nodes for cost-modeling and code-generation. Currently we get away without types, because we mostly rely on the types of the underlying IR. Once all of code-gen is updated to work on VPValues I think we can start to think/work on this transition.

> - Should we split VPInterleaveRecipe (and maybe VPWidenMemoryInstructionRecipe) into different load and store recipes?

At the momentI think the current break-down mirrors the existing code-generation functions in LV (e.g. `vectorizeMemoryInstrution`, `vectorizeInterelaveGroup`). I think that might make sense to break down/split up some of those recipes further in the future, as we move towards migrating the code-gen code, VP cost-modeling & transforms.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88380