[PATCH] D76200: [VPlan] Use underlying value for printing, if available.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, thanks!
Added some minor final comments.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:883
+  const auto *VPI = dyn_cast<VPInstruction>(V);
+  if (!UV && (!VPI || VPI->hasResult()))
+    Slots[V] = NextSlot++;
----------------
Right. May be easier to read if reversed into an early-exiting

```
if (UV || (VPI && !VPI->hasResult()))
  return;
```

At some point it would be good to name VPValues (w/o underlying Values). These would need SlotTracker's attention, to break name reuses.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:739
+
+  bool hasResult() const {
+    if (const Value *UV = getUnderlyingValue())
----------------
fhahn wrote:
> Ayal wrote:
> > Better look at the VPInstruction's Opcode to determine if it has a result?
> I wanted to avoid to have such a check here initially, but it does not look too bad. Unless I missed an instruction, I think calls should be the only instruction where the opcode does not determine if it has a result or not. But just assuming calls have results should be fine.
> 
> Plenty of instructions without result are not likely to be VPInstructions, but I guess it does not hurt to include them here.
Sure. Some of these cannot be processed by LV.

Would be good to switch back to something like "!getType()->isVoidTy()", once [VP]Types are introduced into VPlan/VPValue.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1479
     assert(!Value2VPValue.count(V) && "Value already exists in VPlan");
     Value2VPValue[V] = new VPValue();
   }
----------------
fhahn wrote:
> Ayal wrote:
> > Better record the underlying ingredient to be used when printing, by doing `new VPValue(V)` instead.
> Also better as a follow up?
Very well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76200