[PATCH] SubRegister liveness tracking

[Vincent Lejeune]

Thank for this serie Matthias !
I think it make my patch on "handleMoveUp/Down" obsolete. Do you think the 3 patches set will land before 3.4rc is branched ?
I like to make R600 target support subreg liveness for scheduling in 3.4, on top of your changes.

Vincent





>________________________________
> De : Matthias Braun <mbraun at apple.com>
>À : Renato Golin <renato.golin at linaro.org> 
>Cc : Commit Messages and Patches for LLVM <llvm-commits at cs.uiuc.edu> 
>Envoyé le : Mardi 8 octobre 2013 19h10
>Objet : Re: [PATCH] SubRegister liveness tracking
> 
>
>
>
>
>On Oct 8, 2013, at 7:30 AM, Renato Golin <renato.golin at linaro.org> wrote:
>
>First comments about 0001:
>>
>>
>>CopyFrom seems to assume the dest range is empty, or that there isn't any duplicates between the two (this and other), is that assumption safe on all accounts? If it is, shouldn't there be an assert? If not, shouldn't there be a collision detection?
>Yes CopyFrom expected an empty LiveRange, I changed it to a copy constructor to make that fact more clear.
>
>
>>
>>The SingleLinkedListIterator<> seems a lot of boilerplate on the wrong place. Isn't there another container in LLVM that can do that? If not, shouldn't this code be in a more general header?
>There are only double linked lists available in llvm. Which in this case where you typically have < 3 subranges is overkill IMO.
>
>
>If you think it is useful to others then I will factor the code out and turn it into a “real” C++ ADT similar to the existing ilist, I didn’t do this because it will bloat the code with traits and more overloaded operators for a single use site which doesn’t use them...
>
>
>>
>>You have good comments, but they're not consistent. Some have \brief, others don't. Also, not all new functions have comments. While some functions are obvious, it serves the purpose of better Doxygen pages.
>I updated/added some comments. Missing \briefs shouldn’t hurt I think, I only add \brief when I actually have something to say for the detailed description. Otherwise I assume the brief and detailed docu are just the same.
>
>
>>
>>+void LiveInterval::RemoveEmptySubRanges() {
>>+  while (SubRanges != 0 && SubRanges->empty())
>>+    SubRanges = SubRanges->Next;
>>
>>
>>This looks like an over-optimization of special cases, that would probably be dealt with by the second loop, which is also over-complicated. If all you need is to jump the Next pointer when the SubRange is NULL, you can do that in a single loop, with less confusion, and less premature optimization.
>You are right the implementation of that function was strange and it was missing documentation. After some more thinking I realized that I also don’t need/want that function and simply removed it :)
>
>
>>
>>For my education, SubRange::LaneMask is one bit per lane, right? So, 0x00000011 is the same as D0 or S0+S1, right?
>Well tablegen assigns the LaneMasks to sub register indices (for me this is: ssub_0=0x4, ssub_1=0x8, dsub_0=0xC, dsub_1=0x30, qsub_0=0x3C, …) If there are more than 31 subregister indices all remaining ones will be mapped to bit 32.
>A subregister index like ssub_0 is a more abstract concept only after a concrete register like D0 or D7 is given you can get a concrete subregister S0/S14.
>LaneMasks allow to identify “subtrees” (well subgraphs) in the subregister index hierarchy, which in practice means I can keep the representation at the granularity level of the “DXX” registers if no one is accessing S subregisters.
>
>
>
>>
>>The crucial function verifyLiveInterval() et al. seem ok, though I confess I haven't checked for all possible corner cases. I suspect you'll have tests for all of them.
>>
>>
>>cheers,
>>—renato
>
>
>Thanks
>Matthias
>
>
>>
>>
>>On 8 October 2013 15:00, Renato Golin <renato.golin at linaro.org> wrote:
>>
>>Hi Mathias,
>>>
>>>
>>>I'm not sure how's the best way to review your patches. There are too many, and they're all related...
>>>
>>>
>>>Would that be better on Phabricator? GitHub had a good way to review a series of patches on the same pull request, not sure Phabricator does the same thing.
>>>
>>>
>>>I'm looking at them in order, but the emails will soon entangle when we start reviewing comments from different patches...
>>>
>>>
>>>cheers,
>>>--renato
>>
>
>
>_______________________________________________
>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/20131008/d36aa1b5/attachment.html>