<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;"><div>After a bunch of spelunking, it seems that the miscompile is not in the previous patch itself but is rather a bug in LLVM in how dynamic allocas are handled. </div><div><br></div><div>My reasoning for concluding as such is that I was able to eliminate the previous version of the bug by using:</div><div><br></div><div>1. fastisel on all basic blocks.</div><div>2. selectiondag on all basic blocks except for one very fishy basic block.</div><div><br></div><div>Nick and I chatted on IRC and decided that given that my patch does not expose said issue that I should file a PR with all of the information from my spelunking and proceed with committing this patch. I will post the PR before I commit.</div><div><br></div><div>Michael</div><br><div><div>On Jul 8, 2013, at 4:28 PM, Michael Gottesman <<a href="mailto:mgottesman@apple.com">mgottesman@apple.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;"><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;"><div dir="ltr"><br class="Apple-interchange-newline">I'd like to know what correctness bug was fixed in this patch relative to the patch it's based on. The alternative is to declare that the patch was correct before and merely triggering some other bug in llvm which has since gone away.</div></div></blockquote><div><br></div><div>The latter is the wrong approach. I am going to spend some time this afternoon/evening spelunking a bit more through the miscompiled code. I do not want to commit this until I am able to get to the bottom of the issue.</div><br><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;"><div dir="ltr"><div><br></div><div>I did some testing with this patch applied and didn't encounter any miscompiles. Michael, please test the patch and ensure it doesn't still miscompile some part of the static analyzer (that's what got it reverted last time?).</div></div></div></blockquote><div><br></div><div>I did before I sent it to the list. Specifically the miscompile occurred in the bootstrapped clang when it was running the static analyzer tests.</div><div><br></div><div>Michael</div><br><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;"><div dir="ltr"><div><br></div><div>The patch looks fine to me.<br><div><br></div></div><div>Nick</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 5 July 2013 15:33, Michael Gottesman<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:mgottesman@apple.com" target="_blank">mgottesman@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><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;">Hello llvm-commits!<br><br>This patch teaches TRE how to understand/handle no capture allocas in order<br>to allow for more call sites to have the tail marker placed on them by the TRE pass.<br><br>Without the changes introduced into this patch, if TRE saw any allocas at all,<br>TRE would not perform TRE *or* mark callsites with the tail marker.<br><br>Because TRE runs after mem2reg, this inadequacy is not a death sentence. But<br>given a callsite A without escaping alloca argument, A may not be able to have<br>the tail marker placed on it due to a separate callsite B having a write-back<br>parameter passed in via an argument with the nocapture attribute.<br><br>Assume that B is the only other callsite besides A and B only has nocapture<br>escaping alloca arguments (*NOTE* B may have other arguments that are not passed<br>allocas). In this case not marking A with the tail marker is unnecessarily<br>conservative since:<br><br> <span class="Apple-converted-space"> </span>1. By assumption A has no escaping alloca arguments itself so it can not<br> access the caller's stack via its arguments.<br><br> <span class="Apple-converted-space"> </span>2. Since all of B's escaping alloca arguments are passed as parameters with<br> the nocapture attribute, we know that B does not stash said escaping<br> allocas in a manner that outlives B itself and thus could be accessed<br> indirectly by A.<br><br>With the changes introduced by this patch:<br><br> <span class="Apple-converted-space"> </span>1. If we see any escaping allocas passed as a capturing argument, we do<br> nothing and bail early.<br><br> <span class="Apple-converted-space"> </span>2. If we do not see any escaping allocas passed as captured arguments but we<br> do see escaping allocas passed as nocapture arguments:<br><br> i. We do not perform TRE to avoid PR962 since the code generator produces<br> <span class="Apple-converted-space"> </span>significantly worse code for the dynamic allocas that would be created<br> <span class="Apple-converted-space"> </span>by the TRE algorithm.<br><br> ii. If we do not return twice, mark call sites without escaping allocas<br> with the tail marker. *NOTE* This excludes functions with escaping<br> nocapture allocas.<br><br> <span class="Apple-converted-space"> </span>3. If we do not see any escaping allocas at all (whether captured or not):<br><br> i. If we do not have usage of setjmp, mark all callsites with the tail<br> <span class="Apple-converted-space"> </span>marker.<br><br> ii. If there are no dynamic/variable sized allocas in the function,<br> attempt to perform TRE on all callsites in the function.<br><br>Please Review,<br>Michael<br><br><br><br><br><br>_______________________________________________<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" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></div></div></div></blockquote></div><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;"><a href="mailto:llvm-commits@cs.uiuc.edu" 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;">llvm-commits@cs.uiuc.edu</a><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;"><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" 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;">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></div><br></body></html>