<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><span class=""></span><span class="">Hello </span><span class="">Momchil,</span><div class=""><br class=""></div><div class="">(CC’ing more people that could correct me if I’m wrong)<br class=""><div class=""><br class=""></div><div class="">Thanks for looking into this. More answers below:<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 9 Apr 2018, at 17:57, Momchil Velikov via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class="gmail_default" style="font-family: monospace, monospace;"><font size="2" class="">Hello,<br class=""><br class="">So, I have this testcase:<br class=""><br class=""> void f(int n, int x[]) {<br class=""> if (n < 0)<br class=""> return;<br class=""><br class=""> int a[n];<br class=""><br class=""> for (int i = 0; i < n; i++)<br class=""> a[i] = x[n - i - 1];<br class=""><br class=""> for (int i = 0; i < n; i++)<br class=""> x[i] = a[i] + 1;<br class=""> }<br class=""><br class="">that, compiled with -O1/-Os for AArch64 and X86, generates machine code, which<br class="">fails to properly restore the stack pointer upon function return.<br class=""><br class="">The testcase allocates a VLA, thus clang generates calls to `llvm.stacksave` and<br class="">`llvm.stackrestore`. The latter call is lowered to `mov sp, x2`, however this<br class="">move does not affect decisions made by the shrink wrapping pass, as the<br class="">instruction does not use or define a callee-saved register.<br class=""></font></div></div></div></blockquote><div><br class=""></div><div>I was afraid that this would happen at some point, but never came across a miscompile due to this. I guess the assumption here was that all the code using the stack pointer is generated after/during PEI, so tracking FrameIndex operands would be sufficient.</div><br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_default" style="font-family: monospace, monospace;"><font size="2" class=""><br class="">The end effect is that placement of register save/restore code is such that<br class="">along a certain path, the callee-saved registers and the stack pointer are<br class="">restored, but then the stack pointer is overwritten with an incorrect value.<br class=""><br class="">I've "fixed" this by modifying `ShrinkWrap::useOrDefCSROrFI` to explicitly check<br class="">for the stack pointer register (using<br class="">`TLI.<wbr class="">getStackPointerRegisterToSaveR<wbr class="">estore`)</font></div></div></blockquote><div><br class=""></div><div>This part sounds ok to me. Can you put up a patch please?</div><br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_default" style="font-family: monospace, monospace;"><font size="2" class="">and also to ignore tall call<br class="">instructions (`isCall() && isReturn()`), since they implictly use SP (for<br class="">AArch{32,64} at least).<br class=""></font></div></div></blockquote><div><br class=""></div><div>Calls are handled through the regmask check, and return blocks should be handled by the common-dominator call. Did you run into any issues with this? Marking blocks containing `isReturn` instructions as used will basically make shrink-wrapping to fail all the time.</div><br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_default" style="font-family: monospace, monospace;"><font size="2" class=""><br class="">Does this look correct? Are there alternatives?<br class=""></font></div></div></blockquote><div><br class=""></div><div>Another thing we could do is to add SP (through TLI.getStackPointerRegisterToSaveRestore) to the CurrentCSRs set (and probably rename the set).</div><br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_default" style="font-family: monospace, monospace;"><font size="2" class=""><br class="">Shouldn't `ShrinkWrap::useOrDefCSROrFI` also check whether or not<br class="">`MachineInstr::Frame{Setup,<wbr class="">Destroy}` flags are set?<br class=""></font></div></div></blockquote><div><br class=""></div><div>It’s not perfectly clear what the flag is used for and whether all targets use it for the same purpose. There has been some discussion here: <a href="http://lists.llvm.org/pipermail/llvm-dev/2017-February/110281.html" class="">http://lists.llvm.org/pipermail/llvm-dev/2017-February/110281.html</a>, but I wouldn’t rely on that flag, especially if we want to be able to do some more advanced shrink-wrapping, we are better knowing why the instruction is considered as a “FrameSetup/Destroy” instruction.</div><br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_default" style="font-family: monospace, monospace;"><font size="2" class=""><br class="">In that case, I suppose an alternative slution would be to ensure that<br class="">`llvm.{stacksave,stackrestore} are ultimately lowered to `MachineInstr`s, which<br class="">have these flags set, perhaps by custom lowering of<br class="">ISD::{STACKSAVE,STACKRESTORE} to pseudo-instructions? That'd involve changes to<br class="">each backend, where shrink wrapping is enabled, although it looks least hackish.<br class=""></font></div></div></blockquote><div><br class=""></div><div>I think the SP approach is correct, but yes, this would work as well.</div><div><br class=""></div><div>Thanks,</div><div><br class=""></div><div>— </div><div>Francis</div><br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_default" style="font-family: monospace, monospace;"><font size="2" class=""><br class="">Comments?<br class=""><br class="">Thanks and best regards,<br class="">Momchil Velikov<br class=""><br class=""><br class=""></font></div></div>_______________________________________________<br class="">LLVM Developers mailing list<br class=""><a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<br class=""></blockquote><div class=""><br class=""></div></div></div></div></body></html>