<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;"><br><div><div>On May 30, 2014, at 4:17 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><blockquote type="cite" style="font-family: Helvetica; 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 class="Apple-interchange-newline">On May 30, 2014, at 4:13 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br><br><br><blockquote type="cite">On May 30, 2014, at 3:57 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>> wrote:<br><br><br>On Fri, May 30, 2014 at 3:54 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br><br><blockquote type="cite">On May 30, 2014, at 3:51 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>> wrote:<br><br><br>On Fri, May 30, 2014 at 3:44 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br><br><blockquote type="cite">On May 30, 2014, at 3:40 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><br>On Fri, May 30, 2014 at 3:37 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br><blockquote type="cite"><br><blockquote type="cite">On May 30, 2014, at 3:31 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><br>On Fri, May 30, 2014 at 3:12 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br><blockquote type="cite">On Fri, May 30, 2014 at 3:03 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><blockquote type="cite">On Fri, May 30, 2014 at 3:00 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>> wrote:<br><blockquote type="cite"><br>On Fri, May 30, 2014 at 12:45 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>wrote:<br><blockquote type="cite"><br>On Fri, May 30, 2014 at 10:26 AM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br><blockquote type="cite"><br><blockquote type="cite">On May 29, 2014, at 10:03 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>><br>wrote:<br><br>Yeah, well, we can add "FrameTeardown" machine instruction flag in<br>addition to "FrameSetup", and annotate machine instructions added in frame<br>lowering code. But do we expect many users of this flag (assuming that this<br>usage is also somewhat questionable)?<br></blockquote><br>The FrameSetup flag is also only used by the debug info, so at least it<br>wouldn’t be any more questionable than that :-)<br></blockquote></blockquote><br><br>OK. Why don't we land the patch as-is now (don't terminate the register at<br>the end of MBB if this register is only modified in frame setup and the last<br>MBB - it should be safe), and then replace "the last MBB" condition with<br>checking a "FrameTeardown" flag, attached to certain machine instructions?<br>In this way we won't have to special-case certain registers and make<br>assumptions about their constant-ness.<br></blockquote><br>Given the limited nature of this fix (both in the fix's possible scope<br>and intent/purpose) I'm personally sort of inclined to do this by<br>whitelisting certain registers instead, if that were/is possible...<br>seems simpler and still sufficient for the required purpose here.<br><br>Can we easily just test whether a register is the stack/frame pointer?<br>(I actually know so little about this that the difference between<br>those two things isn't straight in my head and I'm not sure which, or<br>both, we need for the common/intended case here)<br><br></blockquote><br>If we need to stack realign we might not be able to do this as easily.<br>I'm not a huge fan of either method, but I think that the assumption<br>that Alexey has might make more sense than whitelisting.<br></blockquote><br>Fair enough.<br><br><blockquote type="cite">My logic:<br>Whitelisting involves assuming that various registers won't be used in<br>ways that surprise us and I can think of a few ways where it could<br>happen: stack realignment, inline asm, a smarter stack coloring<br>algorithm.<br></blockquote><br>Can inline asm clobber the frame pointer & we actually tolerate that,<br>find the right variables, etc, when that happens? (how?)<br><br>Can't quite picture the stack coloring you have in mind & Alexey<br>commented on the stack realignment issue.<br><br>The only other idea I'd float would be another simpler solution,<br>though a bit more expressive: just a check to see that the particular<br>register(s) (stack/frame pointers, whichever things it is we actually<br>use to describe the location of stack variables) don't change beyond<br>the prologue, and if not, describe any variables that are relative to<br>that register as having that location for the whole program.<br></blockquote><br>That won’t work for values that are spilled to the stack, because they only live on the stack for part of the function.<br></blockquote><br>Do we get those right currently?<br></blockquote><br>Partially. We get them right until the end of the current basic block.<br><br>The patch should improve this situation by the way:<br><bbX>:<br>DBG_VALUE $rsp, 32 !"var" // "var is spilled on stack.<br>// ... more code ..<br><bbY>:<br>DBG_VALUE $r9, 0, !"var" // "var" is now loaded to "r9".<br><br>Earlier we used to terminate the location of "var" at the end of bbX, but now we won't do<br>this, as we know "rsp" doesn't change in the function.<br></blockquote><br>Yeah, but only if we can detect the Epilogue and ignore the def of $rsp that restores the register at the end of the function.<br><br>Yes. So, how do you feel about the plan suggested above:<br><br>OK. Why don't we land the patch as-is now (don't terminate the register at<br>the end of MBB if this register is only modified in frame setup and the last<br>MBB - it should be safe), and then replace "the last MBB" condition with<br>checking a "FrameTeardown" flag, attached to certain machine instructions?<br>In this way we won't have to special-case certain registers and make<br>assumptions about their constant-ness.<br></blockquote><br>Instead of checking for it being the last basic block we could check for something like (BB.getLastNonDebugInstr()->isReturn()) then we would handle cases with multiple return blocks. The only problem that I see is that the last basic block may also contain non-epilogue code. In fact, the entire function may only be a single basic block. A safer condition might be “register is modified in the prologue && has precisely one non-prologue-def in each returning basic block”?<br>Then again that condition would break on a backend that pops each callee-saved register from the stack one-by-one.<br><br>What about this definition: The epilogue is everything after the last instruction that has a DebugLoc attached to it?<br></blockquote><br style="font-family: Helvetica; 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;"><span style="font-family: Helvetica; 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; float: none; display: inline !important;">Sorry, I realized didn’t actually answer you question :-)</span><br style="font-family: Helvetica; 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;"><span style="font-family: Helvetica; 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; float: none; display: inline !important;">I agree that this is a big improvement, but I’m not convinced that the condition for detecting the epilogue you mention is safe, for the same reasons that Eric mentioned earlier. When we find a safe condition for the epilogue this is good to go. My current favorite is looking at the line table.</span><br style="font-family: Helvetica; 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 style="font-family: Helvetica; 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;"></blockquote><div><br></div><div>Note that tail duplication can, and often does, fire for epilogue code. The epilogue is often neither only in one place nor the last basic block in a function.</div><div><br></div><div>-Jim</div><div><br></div><br><blockquote type="cite"><span style="font-family: Helvetica; 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; float: none; display: inline !important;">-- adrian</span><br style="font-family: Helvetica; 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 style="font-family: Helvetica; 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;"><blockquote type="cite" style="font-family: Helvetica; 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>-- adrian<br><blockquote type="cite"><br><br><blockquote type="cite"><br>But there is no code that would insert DBG_VALUES after they are reloaded again.<br><br><br><blockquote type="cite"><br><blockquote type="cite">[And for (-O0) allocas that are described in the MMI table, we already do what you described].<br></blockquote><br>Right - but the problem Alexey is trying to address is what ASan does,<br>where it makes one big alloca and describes variables as being within<br>that alloca - so the MMI sidetable handling doesn't fire…<br></blockquote><br>Awright.<br><br>Yeah, MMI side table doesn't really work for indirect descriptions, when we need to deref the memory stored at reg+offset.<br><br><br><blockquote type="cite"><br><blockquote type="cite"><br>-- adrian<br><br><blockquote type="cite"><br>But, yeah, that's not /lots/ easier than finding any registers that<br>happen to meet that property.<br><br>- David<br><br><blockquote type="cite"><br>-eric<br><br><blockquote type="cite"><blockquote type="cite"><br><blockquote type="cite"><br><blockquote type="cite"><br></blockquote><br>:)<br><br><blockquote type="cite"><blockquote type="cite">I can also make this patch even less general and just special-case the<br>frame pointer and the stack pointer registers, so that the code wouldn't<br>pretend to solve a general problem we're facing.<br></blockquote><br>The frame pointer should be stable over the entire function, special<br>casing it seems to be appropriate. I don’t know whether, e.g, stack coloring<br>would cause the stack pointer to be modified in the middle of a function.<br><br></blockquote><br>FWIW it may not _now_ but there's nothing from stopping it either. :\<br><br>-eric<br></blockquote><br><br>--<br>Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br></blockquote></blockquote></blockquote></blockquote></blockquote></blockquote><br><br><br><br>--<br>Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br></blockquote><br><br><br><br>--<span class="Apple-converted-space"> </span><br>Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br></blockquote><br></blockquote><br style="font-family: Helvetica; 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 style="font-family: Helvetica; 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;"><span style="font-family: Helvetica; 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; float: none; display: inline !important;">_______________________________________________</span><br style="font-family: Helvetica; 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;"><span style="font-family: Helvetica; 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; float: none; display: inline !important;">llvm-commits mailing list</span><br style="font-family: Helvetica; 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;"><span style="font-family: Helvetica; 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; float: none; display: inline !important;"><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a></span><br style="font-family: Helvetica; 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;"><span style="font-family: Helvetica; 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; float: none; display: inline !important;"><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></span></blockquote></div><br></body></html>