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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> 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.

Something like this is what I meant, to try and make it more concrete (this is just meant as pseudo code):

  /// VPInterleaveRecipe is a recipe for transforming an interleave group of load
  /// or stores into one wide load/store and shuffles.
  class VPInterleaveRecipe : public VPRecipeBase {
    const InterleaveGroup<Instruction> *IG;
    SmallVector<std::unique_ptr<VPUser>, 4> Members;
  
  public:
    VPInterleaveRecipe(const InterleaveGroup<Instruction> *IG, VPValue *Addr,
                       VPValue *Mask)
        : VPRecipeBase(VPInterleaveSC, nullptr, {Addr}), IG(IG) {
      if (Mask)
        addOperand(Mask);
      // FIXME: Only loads actually need this, but stores need something better for
      // detecting Mask operands.
      for (size_t i = 0; i < IG->getFactor(); i++)
        Members.emplace_back(
            new VPUser(VPValue::VPExtractSC, IG->getMember(i), {this}));
    }
    ~VPInterleaveRecipe() override = default;
  
    VPValue *getExtract(int Idx) const {
      return Members[Idx].get();
    }

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).

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.  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).

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88380