New Iteration of subregister liveness patches

[David Blaikie]

On Tue, Dec 9, 2014 at 5:32 PM, Matthias Braun <matze at braunis.de> wrote:

> Ok,
>
> I just commited the changes to svn trunk. Thanks everyone, esp. Renato and
> Quentin, for the detailed reviews!
>
> SubRegisterLiveness is available as a subtarget switch now, but not
> enabled by any of the targets yet, as most of them need some additional
> fixing/testcase changes which I'll address in subsequent commits.
>
> For the record: I did fix many but not all issues mentioned in the
> reviews. The following is still missing:
>
> - Manually creating a single linked list in the LiveInterval class. The
> alternatives to this are:
>     a) Using the double linked list provided by llvm.
>     b) Turning the manual single linked list into a full featued ADT in
> llvm.
>

Why not std::forward_list ?


>   Using a double linked list where a single linked list is wasteful, so
> I'm against a). Turning the thing into a full-blown ADT template class
> seemed like too much for a single user in the whole llvm codebase, but if
> others would prefer that over the current code I will do that.
> - Sharing the LiveInterval::verify code with the MachineVerifier code.
> This is not as easy as it looks as you need a working MachineVerifier
> instance if you want to use the MachineVerifier elsewhere. Additionally
> that problem isn't introduced by my commits but already existed before... I
> may take a look into how to make the MachineVerifier more flexible to allow
> this, but this will be another patch series.
> - I clarified the comment in the updateLiveIns() function to explain the
> LiveOutData parameter, but did not add explanation to the other LiveOutData
> parameters yet. It seems like a better explanation at the point where the
> LiveOutData class is defined is the way to go instead of explaining the
> intention over and over again for each function having such a parameter.
> Anyway this is still TODO
> - The code duplication at places that match uses/defs to subranges and
> split the subranges on demand is still duplicated at several places in the
> register coalescer and I think even at other places. This is definitely
> TODO although not easy as all the appearances vary slightly.
>
> Greetings
>     Matthias
>
> > On Dec 5, 2014, at 1:23 PM, Quentin Colombet <qcolombet at apple.com>
> wrote:
> >
> >
> >> On Dec 5, 2014, at 1:20 PM, Renato Golin <renato.golin at linaro.org>
> wrote:
> >>
> >> On 5 December 2014 at 19:19, Tom Stellard <tom at stellard.net> wrote:
> >>> I tested these patches a few months ago and there were some regressions
> >>> on R600.  I'm not sure when I'll get a chance to test them again, but I
> >>> am in favor of merging these patches as soon as possible, since sub-reg
> >>> liveness is hidden behind a subtarget flag and it's easier to fully
> test
> >>> once it is in ToT.
> >>
> >> If this is not the default behaviour, and it can be turned on by just
> >> adding a flag, then I don't see why not, after addressing Quentin's
> >> comments. I agree it'll be much easier to test this once it lands.
> >
> > Same here. And I would add, my concerns can be addressed by subsequent
> patches.
> >
> > Cheers,
> > Q.
> >
> >>
> >> cheers,
> >> --renato
> >
> >
> > _______________________________________________
> > 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/20141210/c12a729a/attachment.html>