<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>This patch was reviewed by people who know this part of the code, it solves an important bug, and it is a local fix. Let’s push it to 3.4. </div><div><br></div><div><br></div><div><div>On Nov 19, 2013, at 2:23 PM, Dimitry Andric <<a href="mailto:dimitry@andric.com">dimitry@andric.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div 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; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">On 19 Nov 2013, at 23:16, Nick Lewycky <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>> wrote:<div><blockquote type="cite"><div dir="ltr">On 19 November 2013 12:24, Dimitry Andric<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:dimitry@andric.com" target="_blank">dimitry@andric.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;"><div class="im">On 13 Nov 2013, at 16:31, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>> I *think* this is a fine improvement, but please add<br>><br>> * A comment saying why jmp foo@PLT is not valid or when it would be valid.<br><br></div>Rafael, did you mean a comment in X86ISelLowering.cpp? I think a jmp foo@PLT is only valid if the stack is setup so the callee can correctly pop it, if needed, and the return types are exactly the same?<br><div class="im"><br><br>> * A note in tailcallpic2.ll saying that it could be optimized to a<br>> direct call (no got, no plt). We don't implement ELF's interposition<br>> logic, but then pretend we do during codegen. This patch doesn't need<br>> to fix that, but should note that we could do better.<br><br></div>Ok, that is easily done.<br><div class="im"><br><br>On 13 Nov 2013, at 23:39, Nick Lewycky <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>> wrote:<br><br>> Sorry, I forgot about this thread entirely. I generally don't approve non-trivial changes under lib/Target and lib/CodeGen, I'd rather somebody else with more assembly/object file/backend knowledge took a look.<br><br></div>Nick, do you feel this is now reviewed enough? I think this is a pretty low-risk change in itself, it only disables an optimization that might lead to trouble, and only on x86/GOT-style platforms. Improving some edge cases (like the direct call Rafael mentioned) can easily be done later, but I would really like to get this change in for the 3.4 release. :-)<br></blockquote><div><br></div><div>I don't think it needs any more review from me, I'll let Rafael (or someone else) decide.</div><div><br></div><div>Sorry, but the 3.4 release has been cut. If this is really low risk then we could argue that this is low risk enough to be cherrypicked into 3.4. That would require a sign off from the x86 backend owner (+cc Nadav).</div></div></div></div></blockquote><div><br></div></div>We just branched, it is not set in stone yet, right? :-) This is simply a bug fix that could be added during Phase I testing. If Nadav can have a quick look at it, that would be great. (Attached the fix rebased against r195016, with one of Rafael's additions to test/CodeGen/X86/tailcallpic2.ll)<div><br></div><div>-Dimitry</div><div><div></div></div><span><fix-pr15086-3.diff></span></div></blockquote></div><br></body></html>