<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 30, 2013 at 2:27 PM, Matthias Braun <span dir="ltr"><<a href="mailto:mbraun@apple.com" target="_blank">mbraun@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class="im"><div>On Aug 30, 2013, at 11:12 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="word-wrap:break-word">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><div>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>
</div></div></blockquote><div><br></div><div>Resist the temptation. If it really seems crucial to fix formatting issues while working on a substantial patch series, you can use `git add --patch` to easily put these sorts of changes into separate commits while working, then periodically do an interactive rebase to aggregate the trivial formatting changes ahead of all your other commits (possibly squashing them together along the way) and then directly commit them.</div>
<div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="im"><div><br></div>
<blockquote type="cite"><div style="word-wrap:break-word"><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><div>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>
<div>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>
<div><br></div><div>Thanks for the review,</div><div><span style="white-space:pre-wrap"> </span>Matthias</div><div class="im"><br><blockquote type="cite"><div style="word-wrap:break-word"><div><br></div><div>Thanks,</div>
<div><div>
<div style="font-family:Helvetica;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;word-wrap:break-word">
-Quentin</div>
</div>
<br><div><div>On Aug 29, 2013, at 6:07 PM, Matthias Braun <<a href="mailto:matze@braunis.de" target="_blank">matze@braunis.de</a>> wrote:</div><br><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" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">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" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div><br></div><br>_______________________________________________<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" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>