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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:818
+  const Value *UV = getUnderlyingValue();
+  if (UV && UV->hasName()) {
+    OS << "%vp." << UV->getName();
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Even if UV does not have a name, could we still indicate that VPValue wraps it; possibly by calling UV->printAsOperand()?
> > Done! It required to add special handling for constants and also a bit of code to drop the % printed by printAsOperand, but now all underlying IR values should be printed consistently. A full list of the required changes can be found in the update comment.
> Thanks!
> Could this be simplified, by always printing "ir<X>" where X is whatever printAsOperand provides, which could be ir<%tmp>, ir<%5>, ir<7>, ir<void>,  letting the caller worry about not printing void?
> 
> And for VPValues w/o ingredients, print "vp<%x>" where x is the number assigned by SlotTracker? Currently this should be only BackedgeTakenCount, which ought to be assigned a meaningful name...
Yes that format simplifies things. I think it looks OK as well.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:892
+  const Value *UV = V->getUnderlyingValue();
+  if (!UV || !UV->hasName())
+    Slots[V] = NextSlot++;
----------------
Ayal wrote:
> Suffice to check now instead if `(!UV)`
If V is a VPInstruction, we should also only number instructions with results. Should be fixed


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:739
+
+  bool hasResult() const {
+    if (const Value *UV = getUnderlyingValue())
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76200