[PATCH 1/5] refactor LiveInterval: introduce new LiveRanges class
Matthias Braun
mbraun at apple.com
Fri Aug 30 13:43:01 PDT 2013
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. So I guess we have to live with slightly less informative debug printing if we go with the patchset…
Greetings
Mathias
>
> 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/4a655be4/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: printreg.patch
Type: application/octet-stream
Size: 6483 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130830/4a655be4/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130830/4a655be4/attachment-0001.html>
More information about the llvm-commits
mailing list