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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D84679#2290130 <https://reviews.llvm.org/D84679#2290130>, @dmgreen wrote:

> Hmm. OK. I do strong believe that we should be moving towards def-use chains, so in general I'm glad to see that happening.
>
> I'm less convinced about the multiple inheritance.  That seems like an antipattern. We can likely make it work either way, but can you try and explain why you think that way is better? As opposed to making all VPRecipe's inherit from VPValue from VPRecipeBase? Perhaps I'm too stuck in the "llvm-ir" way of thinking, with VPValue being Value, VPUser being User, VPRecipeBase being Instruction and the individual recipes being the different instructions.

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.

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.

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 have put up D88152 <https://reviews.llvm.org/D88152> and D88153 <https://reviews.llvm.org/D88153>, to try and show the difference between what I came up with for the two approaches. (They are still very rough. Both can probably be made to work, I'm just not sure what the advantage of the multiple inheritance version is)

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84679