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

[Sean Silva]

On Fri, Aug 30, 2013 at 2:27 PM, 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…
>

Resist the temptation. If it really seems crucial to fix formatting issues
while working on a substantial patch series, you can use `git add --patch`
to easily put these sorts of changes into separate commits while working,
then periodically do an interactive rebase to aggregate the trivial
formatting changes ahead of all your other commits (possibly squashing them
together along the way) and then directly commit them.

-- Sean Silva


>
>
> 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…
> 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.
>
> 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
>
>
>
> _______________________________________________
> 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/20130831/c80e23f3/attachment.html>