<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 Aug 30, 2013, at 11:40 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Aug 30, 2013, at 11:27 AM, Matthias Braun <<a href="mailto:mbraun@apple.com">mbraun@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="Apple-interchange-newline">On Aug 30, 2013, at 11:12 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Matthias,<div><br></div><div>I have a couple of minor comments:</div><div>1. Formatting changes do not help reviewing the patch, things like:</div><div><div>- const_vni_iterator vni_end() const { return valnos.end(); }</div><div>+ const_vni_iterator vni_end() const { return valnos.end(); }</div></div><div>Are necessary.</div><div>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.</div></div></blockquote><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">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…</div></blockquote>Fair enough!</div><div><br><blockquote type="cite" dir="auto"><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br></div><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div>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:</div><div><div>- OS << "Clean " << PrintReg(LI->reg) << " updater: " << *LI << '\n';</div><div>[…]</div><div>+ OS << "Clean updater: " << *LR << '\n';</div></div><div><br></div><div>This is fine of course.</div><div>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).</div></div></blockquote><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">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…</div></blockquote>Agreed, that was what I was saying :).</div><div><br><blockquote type="cite" dir="auto"><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">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.</div></blockquote>Sounds good.</div></div></blockquote><div>I put together the attached patch. It’s a good thing to have LiveInterval print the register number and simplifies the code at some places. Unfortunately I realized that it won’t help in the LiveRangeUpdater case, because print() is not a virtual function and adding virtual functions to LiveRanges just for debug info is not a good idea IMO. So I guess we have to live with slightly less informative debug printing if we go with the patchset…</div><div><br></div><div>Greetings</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>Mathias</div><div><br></div><div></div></div></body></html>