<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;">LGTM.<div><br></div><div>Thanks!</div><div>-Jim</div><div><br></div><div><div><div>On Sep 16, 2013, at 10:20 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;">Ping^3?<div><br><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 style=""><div>On Sep 9, 2013, at 9:21 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;">Ping^2?<div><br><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 style=""><div>On Sep 3, 2013, at 9:38 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;">Ping?<br><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 26, 2013, at 1:08 PM, 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;">Hi,<div><br></div><div>After looking into SROA, Nadav and I agreed that it does the right thing.</div><div>Therefore, the initial proposed patch is still the one to be reviewed.</div><div><br></div><div>Thanks for the feedbacks.</div><div><br></div><div>Cheers,<br><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 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;"></div></div></div></div>
<span><InstCombineLoadSlicing.svndiff></span><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;"><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;"></div>

</div>
<br><div><div>On Aug 20, 2013, at 11:23 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;">Any other opinion about instcombine canonicalization vs. SROA/GVN “lowering sequence”?<div><br></div><div>Thanks,<br><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 20, 2013, at 10:20 AM, Nadav Rotem <<a href="mailto:nrotem@apple.com">nrotem@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;"><div>Hi, </div><div><br></div><div>Quentin and I talked off-line and we now agree that SROA should not generate the "wideload-trunc-shift-trunc" sequence. SROA and GVN should generate a simple sequence of loads. We agreed that this is a Lowering sequence that should be moved to SelectionDAG (or one of the other lowering phases). </div><div><br></div><div>Thanks,</div><div>Nadav</div><div><br></div><br><div><div>On Aug 20, 2013, at 9:56 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;">Hi Nadav,<div><br><div><div>On Aug 19, 2013, at 9:43 PM, Nadav Rotem <<a href="mailto:nrotem@apple.com">nrotem@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;">Hi Quentin, <div><br></div><div>Thanks for working on this. I read the original thread from a few days ago and also looked at the patch. It is still not clear to me why we should add an InstCombine optimization to undo the SROA sequence. Why can’t we change SROA ?</div></div></blockquote><div><br></div><div>This patch transforms the code in a canonical form as suggested by Eli. Thus, we assume that the pattern may come from different optimizations.</div><div>In fact, following Sean’s comment, this also happens with GVN and I do not think we want to change all optimizations that generate such patterns.</div><div><br></div><div>What do you think?</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">I don’t understand the profitability check, but it looks like something that we should do late in the lowering phase. </div></blockquote><div>The name of the check may not be well chosen but the idea is to perform the modification only when it produces the canonical form:</div><div>two loads of an arithmetic type what are next to each other in memory.</div><div><br></div><div>What would you suggest?</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Have you considered just emitting two consecutive loads during SROA, and later on generate the trunc+lshr+bitcast pattern ?</div></blockquote><div>No, for the afore mentioned reasons: making a canonical form :).</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Merging two consecutive loads is really easy to do during SelectionDAG (and we actually already merge consecutive loads and stores for some patterns)</div></blockquote><div>Agree, that’s why we limit the slicing to two loads and not more, but maybe you are suggesting we should do that whatever is the type (i.e., do not prevent the transformation to happen for non-arithmetic type), is it what you are suggesting?</div><div><br></div>Thanks for the review.</div><div><br></div><div>Cheers,</div><div>Quentin<br><br><blockquote type="cite" dir="auto"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div>Thanks,</div><div>Nadav</div><div><br><div><div>On Aug 19, 2013, at 5:35 PM, 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;"><div>Hi,</div><div><br></div><div>Here is a patch that implements the slicing of a load into two smaller loads when the elements are next to each other in memory.</div><div>The original motivation as well as the discussion of why we came up with this patch can be found here:</div><div><a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-August/064769.html">http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-August/064769.html</a></div><div><br></div><div><br></div><div>** Motivation **</div><div><br></div><div>To give a bit of context, the motivation was to get rid of truncate and shift right instructions that get in the way of paired load or floating point load.</div><div>E.g.,</div><div>Consider the following example:</div><div>struct Complex {</div><div><span class="Apple-tab-span" style="white-space:pre">       </span>float real;</div><div><span class="Apple-tab-span" style="white-space:pre">  </span>float imm;</div><div>};</div><div><br></div><div>When accessing a complex, llvm was generating a 64-bits load and the imm field was obtained by a trunc(lshr) sequence, resulting in poor code generation, at least for x86.</div><div><br></div><div><br></div><div>** Proposed Solution **</div><div><br></div><div>Eli Friedman suggested that:</div><div>“[...] two scalar loads at a constant offset from each other are pretty easy to detect for the sorts of passes that like to mess with loads.  So we probably just want to declare that two load instructions is the canonical form for loading two [arithmetic type] which are next to each other in memory, and do this transform on IR for all targets.[…]”</div><div><br></div><div>This patch implements that solution as an instcombine transformation. Also as Eli suggested too, it makes sure the generated loads do not overlap.</div><div><br></div><div>The patch includes a test case that reproduces the initial motivating test case (an add of  complexes), plus a bunch of stress tests that check that the (extreme) slicing is correct for big endian and little endian targets.</div><div><br></div><div><br></div><div>** Performance **</div><div><br></div><div>I have attached the performance numbers for x86-64 (macbook pro core i7 @2.9Hz) for the llvm test-suite.</div><div>I only reported the numbers were the transformation was actually applied.</div><div><br></div><div>The raw numbers where the transformation applied can be found in:</div><div>mod.report.simple.txt: for llvm r188676.</div><div>mod.report.simple.with: for llvm r188676 + this patch.</div><div><br></div><div>Also mod.comparison.txt compares these numbers for tests that ran for at least 1s (otherwise it is too noisy), lower is better.</div><div><br></div><div>The few regressions reported are false positive, i.e., I reran the tests and it is just that the runtime numbers are not that precise.</div><div><br></div><div>Thanks for the feedbacks.</div><div><br></div><div>Cheers,<br><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>
</div></div><span><mod.comparison.txt></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div></div></div>
<span><mod.report.simple.txt></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div></div></div>
<span><mod.report.simple.with></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div></div></div>
<span><InstCombineLoadSlicing.svndiff></span><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div></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><br></blockquote></div><br></div></div></blockquote></div><br></div></div></blockquote></div><br></div></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><br></blockquote></div><br></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><br></blockquote></div><br></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><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><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>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote></div><br></div></body></html>