[PATCH] D84680: [VPlan] Use VPValue def for VPMemoryInstructionRecipe.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:919
 
   VPValue *Mask = getMask();
   if (Mask) {
----------------
dmgreen wrote:
> This looks like it will print mask twice.
Oh right, that's not need any more.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:696
+  /// otherwise.
+  Instruction *getUnderlyingInstr() {
+    if (auto *VPV = toVPValue())
----------------
dmgreen wrote:
> Hmm. I had not considered exposing getUnderlyingInstr at this level. I suppose that should work. Not all recipes with have Instructions and some of them might want different accessors returning the correct type (Phi's or GEP's etc).
> 
> The cast<Instruction> below should be dyn_cast_or_null<Instruction> if the method is expected to return nullptr.
> 
> There is a comment above getUnderlyingValue that says
> ```
>   // DESIGN PRINCIPLE: Access to the underlying IR must be strictly limited to
>   // the front-end and back-end of VPlan so that the middle-end is as
>   // independent as possible of the underlying IR. We grant access to the
>   // underlying IR using friendship. In that way, we should be able to use VPlan
>   // for multiple underlying IRs (Polly?) by providing a new VPlan front-end,
>   // back-end and analysis information for the new IR.
> ```
> I'm not sure if I agree with that design principle, and wouldn't mind that comment being removed and getUnderlyingValue being exposed as public (it can make matching things like constants easier to recognise). If not then the same would apply here.
> Hmm. I had not considered exposing getUnderlyingInstr at this level. I suppose that should work. Not all recipes with have Instructions and some of them might want different accessors returning the correct type (Phi's or GEP's etc).
> 
> The cast<Instruction> below should be dyn_cast_or_null<Instruction> if the method is expected to return nullptr.

I think this should only be called from inside recipes that actually set the underlying value. I think it's fine to crash/assert otherwise. I think we can even push the responsibility to the caller to only use it for VPValues with an underlying value that is an instruction.

> I'm not sure if I agree with that design principle, and wouldn't mind that comment being removed and getUnderlyingValue being exposed as public (it can make matching things like constants easier to recognise). If not then the same would apply here.

yeah, we should probably at least make those helpers protected, so they can only be used from within recipes. I think the long term goal is to de-couple the vpvalues/recipes so far from IR that we can generate IR without relying on references to the original IR. 

Unfortunately, many recipes rely on some information attached to the original IR instructions (mostly for type info, but some other things too). I think it is beneficial to use VPValue's underlying value for the related IR instruction, as it allows us to get rid of the duplicated recipe-specific IR pointer and we also do not have to worry about the underlying value and other reference to diverge. 

I made the functions protected, so the comment above still kind-of applies (and we were doing the same thing for VPInstruction already).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84680