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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

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

> ... snip ..



> The Members are the Values that the other recipes are connected to. They also become uses of the VPInterleaveRecipe (hence they are VPUsers, which glues together a def-use graph). Everything else is pretty standard, VPRecipeBase inherits from VPUser which inherits from VPValue.
> I feel like either if you are having to make modifications that depend upon def-uses then you are altering VPInterleaveRecipe directly and you probably know how to deal with it's uses, or you are doing something like a replaceAllUsesWith on one of those "Members", which should then just work. There might be other things I'm not thinking about though, and I haven't tried to implement anything particularly on top of this scheme (other than the vmulh code, which did not have to deal with interleaving groups specifically, other than they might be a leaf node).

Ah yes, I missed that this was the proposal with the VPInstruction extract nodes!

> I think that method would be simpler and involve less things like creating new VPValues in the constructors of VPRecipes and storing the extra vectors for the defs. It also mirrors the llvm-ir, which like I said I'm a fan of because of it's familiararity.

Creating the VPValues in all the constructors for the recipes in earlier versions of the patches was indeed suboptimal & ugly. In the latest version of the patch, things have improved though I think. Now all recipes expect VPInterleaveRecipe keep inheriting from VPValue and the registration happens as part of the constructor.

> Take a think about it. Maybe try implementing something like the vmulh code on top of the VPDef code. (Or.. maybe something that actually does something with multi-node recipes).

I spent some time porting the VMULH  patch (D88152 <https://reviews.llvm.org/D88152>) on top of the VPDef system: D91198 <https://reviews.llvm.org/D91198> (this excludes the cost-model & VPInstruction::execute changes, but they should not matter).

The only part that needed slight adjustments was the `recursivelyDeleteUnusedRecipes` implementation, which mostly boils down to iterating over all defined values when checking if the value is dead. I think code dealing only with single-def recipes should work unchanged.

  void VPRecipeBase::recursivelyDeleteUnusedRecipes() {
    if (all_of(defined_values(), [](VPValue *Def) {
          return Def->getNumUsers() == 0;
        }) /* && isSafeToRemove()*/) {
      for (auto *Op : operands()) {
        Op->removeUser(*this);
        if (VPRecipeBase *R = dyn_cast<VPRecipeBase>(Op->getDef()))
          R->recursivelyDeleteUnusedRecipes();
      }
      eraseFromParent();
    }
  }



> If you think it's still the best way to go, I'll happily review those patches. So long as we have though through the options.

I think the ergonomics when dealing with single-def recipes is the same with all options.

The main advantage I see of modeling multi-defs is that we 
(1) can avoid artificial/dummy VPValues; conceptually I think making VPInterleaveRecipe inherit from VPValue is not the right fit, it doesn't produce a value (granted, currently this is broken by stores and loads being handled in the same recipe, but that's easy to change); 
(2) have a uniform way to define & handle multi-defs.

IMO it is quite hard to gauge up-front if the extra complexity is really warranted for a cleaner modeling, but given that the overhead seems minor (to me) I think it would preferable to go with the cleaner and uniform modeling. It would probably also be good to hear @Ayal 's or @gilr 's thoughts on this.

Some things that might be slightly harder when using extra VPINstructions to extract from a multi-def might be moving or predicating multi-def recipes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88380