[PATCH] D73078: [VPlan] Use consecutive numbers to print VPValues instead of addresses.

[Gil Rapaport via Phabricator via llvm-commits]

gilr added a comment.

In D73078#1903249 <https://reviews.llvm.org/D73078#1903249>, @fhahn wrote:

> In D73078#1903177 <https://reviews.llvm.org/D73078#1903177>, @gilr wrote:
>
> > In D73078#1902767 <https://reviews.llvm.org/D73078#1902767>, @fhahn wrote:
> >
> > > In D73078#1902664 <https://reviews.llvm.org/D73078#1902664>, @gilr wrote:
> > >
> > > > > [snip]
> > > > >  I think we also introduce some VPInstructions not connected to any underlying IR during predication and will add more in the future. I think we should do (and need) both, using the IR name when available and the slot tracker otherwise. I think it would be good to have using the underlying IR names as a follow up to this patch. What do you think?
> > > >
> > > > After D74445 <https://reviews.llvm.org/D74445> all VPInstructions can indeed print themselves by either using their IR name or defaulting to SlotTracker/badref. So this patch can be simplified by not propagating the SlotTracker through the print() methods, since non-VPInstruction VPValues can rely on their underlying IR values for printing. Note that for this to work addVPValue must plant the underlying IR value into the generated VPValue as you do in D74695 <https://reviews.llvm.org/D74695>.
> > > >  BackedgeTakenCount would then remain the only user of the current pointer-based naming and consequently of the "where:" clause.
> > >
> > >
> > > Ah I think I got what you are proposing now: just remove the `::print(raw_ostream &O, const Twine &Indent, VPSlotTracker &SlotTracker)` variants taking a SlotTracker, because we can just instantiate it on demand, by just getting the parent plan for the containing VPBB. This means we might have to re-number the plan multiple times when printing the whole plan through VPlanPrinter, but as this is just for debugging (and the scope of a plan should be relatively small) we can adjust that as follow-up, if required.
> >
> >
> > So the recursive print of any VP entity should use a single, up-to-date SlotTracker. This SlotTracker may be generated/assigned when entering the recursion (e.g. on `operator<<`  or print()) and destroyed/invalidated when leaving it. One could optimize later to invalidate only when needed, i.e. on VPlan changes.
>
>
> I think I am missing something.
>
> I think this is exactly what the patch does: when calling print without SlotTracker, it will be created on demand and VPlanPrinter re-uses the same slot tracker for printing all VP entities by passing it to print(). I'm not sure what I should change, besides a follow-up to use the underlying values, if available/


Right.

> 
> 
>> VPlanPrinter can also reset the printed Plan's SlotTracker upon start and invalidate it when done. We'll have to make VPlan's SlotTracker `mutable` for this to work, right?
> 
> hmm, IIUC we could add a SlotTracker as member to VPlan, but I am not sure if we should add it, as it is only required for debugging and keeping track of its state might be more work than it's worth.

Agreed.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:636
   /// Each recipe prints itself.
   virtual void print(raw_ostream &O, const Twine &Indent) const = 0;
+  virtual void print(raw_ostream &O, const Twine &Indent,
----------------
It seems all derived recipes have the same implementation for this method, i.e. create a SlotTracker and pass it to the actually-printing variant below. Does it still need to be virtual?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:128
 
-raw_ostream &operator<<(raw_ostream &OS, const VPValue &V);
+// raw_ostream &operator<<(raw_ostream &OS, const VPValue &V);
 
----------------
Can we retain such API, which generates an ad-hoc SlotTracker for VPInstruction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73078