[PATCH] D76200: [VPlan] Use underlying value for printing, if available.
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 17 09:05:29 PDT 2020
Ayal added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:588
}
for (auto Entry : Plan.Value2VPValue) {
OS << "\\n";
----------------
fhahn wrote:
> Ayal wrote:
> > If now every VPValue that wraps a Value clearly says so in its name, does it hopefully make this "%vpA := %irX" part of "where:" redundant?
> I can drop that in a follow-up, if that's ok?
Sure, by all means.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:818
+ const Value *UV = getUnderlyingValue();
+ if (UV && UV->hasName()) {
+ OS << "%vp." << UV->getName();
----------------
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...
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:409
+ if (hasResult())
+ O << " = ";
----------------
Better call `printAsOperand(O, SlotTracker)` only when needed, i.e.,
```
if (hasResult()) {
printAsOperand(O, SlotTracker);
O << " = ";
}
```
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:892
+ const Value *UV = V->getUnderlyingValue();
+ if (!UV || !UV->hasName())
+ Slots[V] = NextSlot++;
----------------
Suffice to check now instead if `(!UV)`
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:739
+
+ bool hasResult() const {
+ if (const Value *UV = getUnderlyingValue())
----------------
Better look at the VPInstruction's Opcode to determine if it has a result?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1479
assert(!Value2VPValue.count(V) && "Value already exists in VPlan");
Value2VPValue[V] = new VPValue();
}
----------------
Better record the underlying ingredient to be used when printing, by doing `new VPValue(V)` instead.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76200/new/
https://reviews.llvm.org/D76200
More information about the llvm-commits
mailing list