[PATCH] D84679: [VPlan] Disconnect VPValue and VPUser.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> The main reason for migrating a recipe at a time is to keep the reviews small so we can make sure we properly update all places in the code that previously referenced the underlying instruction, e.g. during codegen. Another example is VPUser. I think we only want to make `VPRecipeBase` inherit from `VPUser` once *all* recipes manage their operands via `VPUser`. Otherwise you cannot rely on the VPUser operands being the source of truth.

Splitting things up sounds very smart. My question was less about doing that and more about whether we are heading towards the correct final design. And trying to show how that might look, when we work it though in a real problem. After all, the longer we go on using it, the harder it will become to change.

We could consider removing the VPUser concept entirely, for example. If there is only ever one thing that inherits from it (unlike llvm's User), the Operands could just be could be folded into VPRecipeBase.

> In particular, just making `VPRecipeBase` inherit from `VPValue` without updating the existing code will result in subtle problems, e.g. when updating VPValue operands of a recipe, but codegen still using the original IR references.

Yep. That will certainly be very important for replacing recipes, as the vmulh patch does. Some ways to test some of the invariants would be useful as well.

> Once all recipes are updated and inherit from VPValue we can move it to `VPRecipeBase`. The main caveat is `VPInterleaveRecipe`, which is not really a `VPValue`, but rather something that produces multiple values.  We can also make that a `VPValue`, but not reference that particular value anywhere else, add all users of sub-values to the users of the 'virtual' VPValue and treat it as a convenient way to access all users of all sub-values. That's not completely clean, but we could also extract the user part into something else that can be inherited more cleanly from `VPRecipeBase`

I was thinking of complex nodes too, which may have multiple outputs. It will depend on how they are represented though, they may just remain as single output nodes, but you match them to operate differently on different lanes.

> Thanks for sharing. As mentioned earlier, the main reason at the moment is to ensure a step-by-step transition and once all recipes are migrated the common class can be hoisted to `VPRecipeBase`. I hope the explanation makes sense.

Like I said I'm happy that we are moving towards the ability to do this better. It will be good to see VPlan being able to start to make some real improvements! If we are happy enough with this way of doing things, at least currently, then I can go and make some more sensibly sized patches :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84679