[PATCH 1/5] refactor LiveInterval: introduce new LiveRanges class

[Quentin Colombet]

On Aug 30, 2013, at 1:43 PM, Matthias Braun <mbraun at apple.com> wrote:

> 
> On Aug 30, 2013, at 11:40 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> 
>> 
>> On Aug 30, 2013, at 11:27 AM, Matthias Braun <mbraun at apple.com> wrote:
>> 
>>> 
>>> On Aug 30, 2013, at 11:12 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>> 
>>>> Hi Matthias,
>>>> 
>>>> I have a couple of minor comments:
>>>> 1. Formatting changes do not help reviewing the patch, things like:
>>>> -    const_vni_iterator vni_end() const { return valnos.end(); }
>>>> +    const_vni_iterator vni_end() const   { return valnos.end(); }
>>>> Are necessary.
>>>> Note that I do not know the LLVM policy for this kind of thing, but I am not fond of additional space stuff to align block.
>>> I general I tried to keep unnecessary formatting changes out of the patches. At this place I did it anyway, because at least one of the small functions already had 2 spaces instead of 1 but for some strange reason not enough to match up the braces correctly…
>> Fair enough!
>> 
>>> 
>>>> 
>>>> 2. Because of the refactoring, the LiveRange class does not have the register field anymore, thus you remove the reference in the print method of the LiveRangeUpdater:
>>>> -      OS << "Clean " << PrintReg(LI->reg) << " updater: " << *LI << '\n';
>>>> […]
>>>> +      OS << "Clean updater: " << *LR << '\n';
>>>> 
>>>> This is fine of course.
>>>> However, I think you should also modify LiveInterval::print to print the register to ease debug purpose because now, we have less information (unless I missed something).
>>> Well after the patch the LiveRangeUpdater can operator on any liveranges not just for the vreg liveranges anymore, so it’s not really possible to show a sensible register number in each case and I didn’t want to push context like a register number around just to improve the debug output…
>> Agreed, that was what I was saying :).
>> 
>>> Always outputting the register number in LiveInterval::print would probably be a good idea but it would need some refactoring because there are lots of places in the code where it first prints the register number and the interval and I don’t want to see that info twice. I’ll see if I can create an extra patch for that.
>> Sounds good.
> I put together the attached patch. It’s a good thing to have LiveInterval print the register number and simplifies the code at some places. Unfortunately I realized that it won’t help in the LiveRangeUpdater case, because print() is not a virtual function and adding virtual functions to LiveRanges just for debug info is not a good idea IMO.
Another option is to do the same kind of thing we have been done in Value, i.e., the generic class dispatches the printing to the right method according to the type.
That is not a very good OO design but it does the trick.

I am not sure we want to go in that direction, although it may be painful not having this information for debug.

Side remark, in the MachineVerifier, I think we may want to keep those messages because the LiveInterval cannot print that information (it does not have a TRI) and I guess it is not too bad to have the register printed twice.
-  *OS << "- interval:    ";
-  if (TargetRegisterInfo::isVirtualRegister(LI.reg))
-    *OS << PrintReg(LI.reg, TRI);
-  else
-    *OS << PrintRegUnit(LI.reg, TRI);
-  *OS << ' ' << LI << '\n';
+  *OS << "- interval:    " << LI << '\n';

What do you think?

-Quentin
> So I guess we have to live with slightly less informative debug printing if we go with the patchset…
> 
> Greetings
> 	Mathias
> 
> <printreg.patch>
> 
>> 
>> Thanks,
>> -Quentin
>> 
>>> 
>>> Thanks for the review,
>>> 	Matthias
>>> 
>>>> 
>>>> Thanks,
>>>> -Quentin
>>>> 
>>>> On Aug 29, 2013, at 6:07 PM, Matthias Braun <matze at braunis.de> wrote:
>>>> 
>>>>> 
>>>>> LiveRanges has logic to manage a set list LiveRange instances
>>>>> and a list of value numbers which previously was in LiveInterval,
>>>>> without having details like spill weight or a fixed register number.
>>>>> LiveInterval is now a direct subclass of LiveRanges and simply adds the
>>>>> spill weight and the register number.
>>>>> ---
>>>>> include/llvm/CodeGen/LiveInterval.h  | 143 +++++++++++++++++++++--------------
>>>>> lib/CodeGen/LiveInterval.cpp         | 128 +++++++++++++++----------------
>>>>> lib/CodeGen/LiveIntervalAnalysis.cpp |  20 ++---
>>>>> 3 files changed, 156 insertions(+), 135 deletions(-)
>>>>> 
>>>>> <0001-refactor-LiveInterval-introduce-new-LiveRanges-class.patch>_______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> 
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130830/c6533c3f/attachment.html>