
<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 Jun 4, 2013, at 1:29 PM, David Majnemer <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.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;"><div dir="ltr">It isn't all divide operations. Just the ones that we can't prove have a non-zero denominator.<div><br></div><div>I think that the proper thing to do is to stick in in the loop preheader.</div></div></div></blockquote><div><br></div>I don’t think we want to stick anything in the preheader in this case.</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;"><div dir="ltr"><div>However, many other pieces of code see that it is loop invariant and hoist it out of said loop preheader.</div><div>I'll give it a shot later.</div></div></div></blockquote><div dir="auto"><br></div>Yes, LICM takes care of hoisting loop invariant divides. Rafael showed that the case you disabled is not worth optimizing.</div><div><br></div><div>I think the only better fix here is to rewrite RewriteLoopExitValues so it does not depend on SCEVExpander (major undertaking). Trying to embed IR rewriting constraints inside SCEV and messing with insertion points is horrible. I’d prefer to just sink a chain of instructions and let IR utilities reduce the expression. If we want to hoist instructions to hide latency an MI pass should do that.</div><div><br></div><div>Thanks for the fix!</div><div><br></div><div>-Andy</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;"><div dir="ltr"><div><br></div><div>-- </div><div>David Majnemer</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jun 4, 2013 at 8:48 AM, James Molloy<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</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;"><div class="im">> <span style="color: rgb(34, 34, 34); font-size: 13px; font-family: arial, sans-serif;">The only case this could regress is a potentially trapping operation that doesn't actually trap for reasons that are invisible to the optimizer.</span><div><font color="#222222" face="arial, sans-serif"><br></font></div></div><div><font color="#222222" face="arial, sans-serif">Like a divide operation where the divisor never happens to be zero? or am I misunderstanding the patch?</font></div><div><font color="#222222" face="arial, sans-serif"><br></font></div><div><font color="#222222" face="arial, sans-serif">My understanding is that now we will not hoist divides out of induction variable expressions where before we did, because divides can trap. Most divides in user code don't trap, so I don't see how you can conclude that the optimization is not firing on user code.</font></div><div><font color="#222222" face="arial, sans-serif"><br></font></div><div><font color="#222222" face="arial, sans-serif">That said, I agree let's fix the crash with the trivial fix proposed and deal with the performance impact later.<br></font><div><div class="h5"><br><div class="gmail_quote">On 4 June 2013 16:41, Rafael Ávila De Espíndola<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.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;"><div dir="auto"><div>We are more likely to fix a crash. So I don't think it is firing. The only case this could regress is a potentially trapping operation that doesn't actually trap for reasons that are invisible to the optimizer.</div><div><br></div><div>Adding the extra control flow to guard it is not clear to be a win. We should probably fix the miscompilation first. If we do have a perf regression somewhere, it can be addressed in a subsequent patch.<br><br>Sent from my iPhone</div><div><div><br>On 2013-06-04, at 11:22, James Molloy <<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a>> wrote:<br><br></div><blockquote type="cite">Hi Rafael,<div><br></div><div>I don't know. Do we know this broken optimization doesn't kick in in the test suite? If it does, we'd regress on performance if we took the conservative way out.</div><div><br></div><div>Just my 2c.</div><div><br></div><div>Cheers,</div><div><br></div><div>James<br><br><div class="gmail_quote">On 4 June 2013 15:40, Rafael Espíndola<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.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;"><div>On 4 June 2013 09:49, James Molloy <<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a>> wrote:<br>> Hi David,<br>><br>> Might a more performant solution not be to wrap any hoisted instructions<br>> that are not safe to expand in a conditional that tests if the loop will be<br>> entered at least once?<br><br></div>Is that really worth it? Given that this bug went undetected for years<br>(it was already broken in r122610), I don't think it would kick often<br>enough to justify it.<br><br>Cheers,<br>Rafael<br></blockquote></div><br></div></blockquote></div></div></blockquote></div><br></div></div></div></blockquote></div><br></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</div></blockquote></div><br></body></html>