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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:919
 
   VPValue *Mask = getMask();
   if (Mask) {
----------------
This looks like it will print mask twice.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:696
+  /// otherwise.
+  Instruction *getUnderlyingInstr() {
+    if (auto *VPV = toVPValue())
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84680