<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;">On Aug 30, 2013, at 1:43 PM, Matthias Braun <<a href="mailto:mbraun@apple.com">mbraun@apple.com</a>> wrote:<br><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;"><div><br class="Apple-interchange-newline">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"><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.</div></div></blockquote><div>Another option is to do the same kind of thing we have been done in Value, i.e., the generic class dispatches the printing to the right method according to the type.</div><div>That is not a very good OO design but it does the trick.</div><div><br></div><div>I am not sure we want to go in that direction, although it may be painful not having this information for debug.</div><div><br></div><div>Side remark, in the MachineVerifier, I think we may want to keep those messages because the LiveInterval cannot print that information (it does not have a TRI) and I guess it is not too bad to have the register printed twice.</div><div><div>- *OS << "- interval: ";</div><div>- if (TargetRegisterInfo::isVirtualRegister(LI.reg))</div><div>- *OS << PrintReg(LI.reg, TRI);</div><div>- else</div><div>- *OS << PrintRegUnit(LI.reg, TRI);</div><div>- *OS << ' ' << LI << '\n';</div><div>+ *OS << "- interval: " << LI << '\n';</div></div><div><br></div><div>What do you think?</div><div><br></div>-Quentin<br><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;"><div>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><span><printreg.patch></span><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;"><div></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div>Thanks,</div><div><div apple-content-edited="true"><div style="orphans: 2; text-align: -webkit-auto; widows: 2; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">-Quentin</div><div><br></div></div><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><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;">Thanks for the review,</div><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;"><span class="Apple-tab-span" style="white-space: pre;"> </span>Matthias</div><br 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;"><div><br></div><div>Thanks,</div><div><div apple-content-edited="true"><div style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">-Quentin</div></div><br><div><div>On Aug 29, 2013, at 6:07 PM, Matthias Braun <<a href="mailto:matze@braunis.de">matze@braunis.de</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><br>LiveRanges has logic to manage a set list LiveRange instances<br>and a list of value numbers which previously was in LiveInterval,<br>without having details like spill weight or a fixed register number.<br>LiveInterval is now a direct subclass of LiveRanges and simply adds the<br>spill weight and the register number.<br>---<br>include/llvm/CodeGen/LiveInterval.h | 143 +++++++++++++++++++++--------------<br>lib/CodeGen/LiveInterval.cpp | 128 +++++++++++++++----------------<br>lib/CodeGen/LiveIntervalAnalysis.cpp | 20 ++---<br>3 files changed, 156 insertions(+), 135 deletions(-)<br><br><span><0001-refactor-LiveInterval-introduce-new-LiveRanges-class.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></blockquote></div><br></div></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></blockquote></div></div></blockquote></div></blockquote></div><br></body></html>