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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:69
+//  Virtual VPValues are used to model instructions/recipes that either produce
+//  multiple subvalues or no values at all. A virtual VPValue does not refer to
+//  a concrete value, which means it cannot be used like concrete or subvalues.
----------------
dmgreen wrote:
> "no values at all" could be concrete VPValues, just with void return type.
If we have recipes that exclusively have void return types, 'virtual' values could indeed be used. The problem with the current recipes unfortunately is that for example `VPWidenMemoryInstructionRecipe` combines both load & store instructions and they share the same SubclassID. Same for others, like `VPWidenCall`. Not sure if we would create plain VPValues with void return type in practice.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:129
     VPValueSC,
+    VPVirtualValueSC,
+    VPMultiValueSC,
----------------
dmgreen wrote:
> When would VPVirtualValueSC be used?
This is only used for testing. Alternatively I could just keep VPInterleaveSC in this patch and use that instead.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:136
+    VPVWidenGEPSC,
+    VPVInterleaveSC
   };
----------------
This should be part of D84684


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:158-159
+    Users.push_back(&User);
+    if (isSubValue())
+      getDefiningValue()->Users.push_back(&User);
+  }
----------------
dmgreen wrote:
> The defining recipe would be another user of the value? The sounds like it would complicate the number of uses. When would it be useful to store this in both places?
> The defining recipe would be another user of the value? 

As is yes. The main advantage is that this would allow clients to traverse the def-use chains without necessarily needing to account for 'virtual' values directly. A cleaner alternative would be to account for that when the client asks for the list of users and combine the users of all sub-values for virtual values on demand.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:129
+    VPVirtualValueSC,
+    VPMultiValueSC,
     VPInstructionSC,
----------------
dmgreen wrote:
> Would a VPMultiValueSC ever be used, or would it always be a  VPVInterleaveSC? (Or whatever other recipe type it became)
This is left over from a previous version. I'll remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88380