<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Jun 6, 2013, at 5:00 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: 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; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi,<div><br></div><div>** Context **</div><div>Recently lifetime markers have been generated by clang in the IR to be able to take advantage of the stack coloring pass.</div><div>When these markers are set, the stack coloring pass changes, among other things, the value (setValue) attached to MachineMemoryOperand. In particular, this optimization is setting the value to 0 (<unknown>) when it cannot determine that the value does not alias with a merged alloca (StackColoring.cpp:540).</div><div><br></div><div>In the ARM backend, the load/store optimizer checks the presence of this value to decide whether it should form paired load or paired store.</div><div><br></div><div>Therefore, the stack coloring pass disables the capabilities of the ARM backend to form paired load or paired store before register allocation.</div><div><<a href="rdar://problem/13978317">rdar://problem/13978317</a>></div><div><br></div><div>** Proposed Solution **</div><div>The attached patch removes the dependence of the ARM backend on the value (in fact, this was a false dependency, at least from what I understood).</div><div>In particular, it does not change the stack coloring pass, as setting the value to 0 seems the right thing to do for this analysis.</div><div><br></div><div>** Test Cases **</div><div>The patch adds one test case of a paired load formed when lifetime marker are present (prior to this patch, the paired load is not formed for this test case).</div><div>The patch also modifies <span style="font-family: Menlo; font-size: 11px;">test/CodeGen/ARM/2012-10-04-AAPCS-byval-align8.ll.</span> Indeed, because of the patch, in that test, we are able to form a paired load before register allocation. Because of a bad interaction with the coalescer, the register allocator is not able to set a proper pair for the paired load. This is causing the pair loaded to be break apart after register allocation.</div><div>Prior to this patch, the pair loaded was caught after register allocation, because both copies were coalesced. Anyway, the register allocator has not clue it may form a paired load when coalescing those.</div><div>If the patch is accepted, I will file an enhancement request against the register allocator.</div><div><br></div><div>** Action Required **</div><div>- For Stack coloring pass expert: Confirm that the pass cannot preserve the value of the MachineMemoryOperand. I think the dependence of the ARM back on that is artificial but if we can preserve some information, why not!</div></div></blockquote><div dir="auto"><br></div>I agree with Nadav that we cannot preserve the IR value. I would only add that we really want to preserve MachineMemOperands in this case, but it would require reimplementing stack coloring in IR.</div><div><br><blockquote type="cite"><div style="letter-spacing: 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; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>- For ARM backend expert: Confirm that the value is not required. Or put differently, what was the purpose checking if a value was set?</div></div></blockquote><div><br></div>AFAICT, the value is not currently required. It will be required when we decide to use AliasAnalysis in the LoadStoreOptimizer.</div><div><br></div><div>Essentially, if we ever want to rely on AliasAnalysis after StackColoring, we need to reimplement StackColoring at IR level. For now your patch LGTM.</div><div><br></div><div>-Andy<br><br><blockquote type="cite"><div style="letter-spacing: 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; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Thanks for the review,</div><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></div><span><Mail Attachment></span><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></div></blockquote></div><br></body></html>