<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;"><div></div><div>
<br><div><div>On Aug 28, 2013, at 2:54 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="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;">Interesting patch. It could definitely use some numbers.</div></blockquote><div>The patch is not triggered by any of the tests that are in the llvm test suite (tested on both ARM and X86).</div><div><br></div><div>Like I said in my original email, I could rewrite/eliminate more aggressively copies (and in that case it does kick in in the llvm test suite) but then, the register allocator does in a few cases a poor coalescing/live-range splitting job.</div><div><br></div><div>That said, as it is, when the patch kicks in, we observed with our internal tests: no regressions and up to 25% runtime speed-up.</div><div><br></div><blockquote type="cite"><div style="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;">Perhaps an<br>example of what you're optimizing as a comment in the code?<br></div></blockquote><div>I have added one in the file header. What do you think?</div><br><blockquote type="cite"><div style="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>Silly nits:<br><br>couple (RegisterClass, SubReg)<br><br>maybe "pair”?<br></div></blockquote>Done (and thanks ;)).</div><div><br><blockquote type="cite" dir="auto"><div style="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>// if<br><br>Capitalize.<br></div></blockquote>Done (and good catch :)).<br><br><blockquote type="cite" dir="auto"><div style="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>A few more things:<br><br>+ } while (!ShouldRewrite && (Copy = MRI->getVRegDef(Src)) && Copy->isCopy());<br><br>I really dislike assignments in this sort of conditional.<br></div></blockquote><div>I have moved the assignment inside the loop.</div><div>Is it better?</div><div>(I do like predicated code!)</div><br><blockquote type="cite" dir="auto"><div style="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>I'm not necessarily comfortable approving, but the code looks reasonable.<br></div></blockquote></div><div>What do you miss to be comfortable?</div><div>Could you point me someone that may be comfortable to approve this, please? (So that I can through him under the bus!)</div><div><br></div><div>Thanks again.</div><div><br></div><div>-Quentin<br><blockquote type="cite" dir="auto"><div style="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>-eric<br><br><br>On Fri, Aug 23, 2013 at 1:04 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><blockquote type="cite">Hi,<br><br>Here is a patch to rewrite copies to avoid cross register banks copies when possible.<br>Thanks for your reviews.<br><br>** Context **<br><br>By definition copies across register bank are not coalescable. Still, it may be possible to get rid of such a copy when the value is available in another register of the same register file.<br>Consider the following example, where capital and lower letters denote different register file:<br>b = copy A <-- cross-bank copy<br>…<br>C = copy b <-- cross-bank copy<br><br>This could have been optimized this way:<br>b = copy A <-- cross-bank copy<br>…<br>C = copy A <-- same-bank copy<br><br>Note: b and C's definitions may be in different basic blocks.<br><br><br>** Proposed Solution **<br><br>Add a peephole optimization that looks through a chain of copies leading to a cross-bank copy and reuses a source that is on the same register file if available.<br><br>This solution could also be used to get rid of some copies (e.g., A could have been used instead of C). However, we do not do so because:<br>- It may over constrain the coloring of the source register for coalescing.<br>- The register allocator may not be able to find a nice split point for the longer live-range, leading to more spill.<br><br><br>** Testcase?! **<br><br>This patch does not include a test case, because it is very difficult to reproduce this behavior with a reasonably small input.<br><br>Cheers,<br>-Quentin<br><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">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></div></blockquote></div><br></div></body></html>