<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Oct 8, 2013, at 7:30 AM, Renato Golin <<a href="mailto:renato.golin@linaro.org">renato.golin@linaro.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">First comments about 0001:<div><br></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><blockquote type="cite"><div dir="ltr">
<div><br></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></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><blockquote type="cite"><div dir="ltr">
<div><br></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><blockquote type="cite"><div dir="ltr">
<div><br></div><div><div>+void LiveInterval::RemoveEmptySubRanges() {</div><div>+ while (SubRanges != 0 && SubRanges->empty())</div><div>+ SubRanges = SubRanges->Next;</div><div><br></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><blockquote type="cite"><div dir="ltr">
<div><br></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><br></div><blockquote type="cite"><div dir="ltr"><div><br></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></div><div>cheers,</div><div>—renato</div></div></blockquote><div><br></div><div>Thanks</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>Matthias</div><div></div></div></body></html>