<html><body><div style="color:#000; background-color:#fff; font-family:times new roman, new york, times, serif;font-size:12pt">Thank for this serie Matthias !<br>I think it make my patch on "handleMoveUp/Down" obsolete. Do you think the 3 patches set will land before 3.4rc is branched ?<br>I like to make R600 target support subreg liveness for scheduling in 3.4, on top of your changes.<br><br>Vincent<br><div><span><br></span></div><div><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; margin-top: 5px; padding-left: 5px;"> <div style="font-family: times new roman, new york, times, serif; font-size: 12pt;"> <div style="font-family: times new roman, new york, times, serif; font-size: 12pt;"> <div dir="ltr"> <hr size="1"> <font face="Arial" size="2"> <b><span style="font-weight:bold;">De :</span></b> Matthias Braun <mbraun@apple.com><br> <b><span style="font-weight: bold;">À :</span></b> Renato Golin
<renato.golin@linaro.org> <br><b><span style="font-weight: bold;">Cc :</span></b> Commit Messages and Patches for LLVM <llvm-commits@cs.uiuc.edu> <br> <b><span style="font-weight: bold;">Envoyé le :</span></b> Mardi 8 octobre 2013 19h10<br> <b><span style="font-weight: bold;">Objet :</span></b> Re: [PATCH] SubRegister liveness tracking<br> </font> </div> <div class="y_msg_container"><br><div id="yiv0170240038"><div><br clear="none"><div><div>On Oct 8, 2013, at 7:30 AM, Renato Golin <<a rel="nofollow" shape="rect" ymailto="mailto:renato.golin@linaro.org" target="_blank" href="mailto:renato.golin@linaro.org">renato.golin@linaro.org</a>> wrote:</div><br class="yiv0170240038Apple-interchange-newline" clear="none"><blockquote type="cite"><div dir="ltr">First comments about 0001:<div><br clear="none"></div><div>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?</div></div></blockquote><div>Yes CopyFrom expected an empty LiveRange, I changed it to a copy constructor to make that fact more clear.</div><br clear="none"><blockquote type="cite"><div dir="ltr">
<div><br clear="none"></div><div>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?</div></div></blockquote><div>There are only double linked lists available in llvm. Which in this case where you typically have < 3 subranges is overkill IMO.</div><div><br clear="none"></div><div>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...</div><br clear="none"><blockquote type="cite"><div dir="ltr">
<div><br clear="none"></div><div>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.</div></div></blockquote><div>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.</div><br clear="none"><blockquote type="cite"><div dir="ltr">
<div><br clear="none"></div><div><div>+void LiveInterval::RemoveEmptySubRanges() {</div><div>+ while (SubRanges != 0 && SubRanges->empty())</div><div>+ SubRanges = SubRanges->Next;</div><div><br clear="none"></div></div><div>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.</div></div></blockquote><div>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 :)</div><br clear="none"><blockquote type="cite"><div dir="ltr">
<div><br clear="none"></div><div>For my education, SubRange::LaneMask is one bit per lane, right? So, 0x00000011 is the same as D0 or S0+S1, right?</div></div></blockquote><div>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.</div><div>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.</div><div>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.</div><div class="yiv0170240038yqt5584545085" id="yiv0170240038yqtfd16169"><div><br clear="none"></div><blockquote type="cite"><div
dir="ltr"><div><br clear="none"></div><div>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.</div>
<div><br clear="none"></div><div>cheers,</div><div>—renato</div></div></blockquote></div><div><br clear="none"></div><div>Thanks</div><div><span class="yiv0170240038Apple-tab-span" style="white-space:pre;"> </span>Matthias</div><div class="yiv0170240038yqt5584545085" id="yiv0170240038yqtfd93903"><div></div></div></div></div></div><br><div id="yiv0170240038"><div class="yiv0170240038yqt5584545085" id="yiv0170240038yqtfd43566"><div><div><div></div><blockquote type="cite"><div class="yiv0170240038gmail_extra"><br clear="none"><br clear="none"><div class="yiv0170240038gmail_quote">On 8 October 2013 15:00, Renato Golin <span dir="ltr"><<a rel="nofollow" shape="rect" ymailto="mailto:renato.golin@linaro.org" target="_blank" href="mailto:renato.golin@linaro.org">renato.golin@linaro.org</a>></span> wrote:<br clear="none">
<blockquote class="yiv0170240038gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div dir="ltr">Hi Mathias,<div><br clear="none"></div><div>I'm not sure how's the best way to review your patches. There are too many, and they're all related...</div>
<div><br clear="none"></div><div>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.</div>
<div><br clear="none"></div><div>I'm looking at them in order, but the emails will soon entangle when we start reviewing comments from different patches...</div><div><br clear="none"></div><div>cheers,</div><div>--renato</div></div>
</blockquote></div><br clear="none"></div>
</blockquote></div><br clear="none"></div></div></div><br><div class="yqt5584545085" id="yqtfd55879">_______________________________________________<br clear="none">llvm-commits mailing list<br clear="none"><a shape="rect" ymailto="mailto:llvm-commits@cs.uiuc.edu" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br clear="none"><a shape="rect" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br clear="none"></div><br><br></div> </div> </div> </blockquote></div> </div></body></html>