<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;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Dec 11, 2014, at 12:40 PM, Mark Heffernan <<a href="mailto:meheff@google.com" class="">meheff@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">On Thu, Dec 11, 2014 at 1:26 AM, Andrew Trick <span dir="ltr" class=""><<a href="mailto:atrick@apple.com" target="_blank" class="">atrick@apple.com</a>></span> wrote:<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">Sorry I didn't respond. I wasn't able to easily convince myself that this fixes the problem. Since this reverts to the previous implementation, I think it's a good change. But the code is written in a way that is extremely hard to reason about.<br class=""></blockquote><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">For example:<br class="">- StepV could be negative but still be a power of two, is this expected?<br class=""></blockquote><div class=""><br class=""></div><div class="">What the ispowerof2 function returns isn't totally clear from its name. It returns true if the number as an *unsigned* value is a power of 2, that is, it has exactly one bit set.</div></div></div></div></div></blockquote><div><br class=""></div><div>Except for the INT_MIN corner case, which you’re aware of from your comments.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><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">- Why are we checking GetMinTrailingZeros(getNegativeSCEV(Start)) instead of GetMinTrailingZeros(Distance)?<br class=""></blockquote><div class=""><br class=""></div><div class="">Isn't this expression always true: GetMinTrailingZeros(foo) == GetMinTrailingZeros(getNegativeSCEV(foo)) ? If so, it doesn't matter whether we use Start or </div></div></div></div></div></blockquote><div><br class=""></div>Indeed, which makes it very misleading to call getNegativeSCEV here. I would use “Distance” everywhere that we’re assuming we have a nonnegative value.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">getNegativeSCEV(Start) and distance necessarily is one of those values. Given that, it probably makes sense to use a non-negated form, that is <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important;background-color:rgb(255,255,255)" class="">GetMinTrailingZeros(Start</span><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important;background-color:rgb(255,255,255)" class="">)</span> or GetMinTrailingZeros(Distance) like you suggest.</div><div class=""><br class=""></div><div class="">Looking at the getUdivExpr:</div><div class=""><br class=""></div><div class=""><div class="">const SCEV *Exact =</div><div class=""> getUDivExpr(Distance, CountDown ? getNegativeSCEV(Step) : Step);</div></div><div class=""><br class=""></div><div class="">This is unnecessarily obfuscated. It is equivalently:</div><div class=""><br class=""></div><div class=""><div class="">const SCEV *Exact =</div><div class=""> getUDivExpr(CountDown ? Start : getNegativeSCEV(Start), CountDown ? getNegativeSCEV(Step) : Step);</div></div><div class=""><br class=""></div><div class="">Seems like this can be expressed as:</div><div class=""><br class=""></div><div class=""><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255)" class=""><div class="">const SCEV *Exact = getUDivExpr(Start, getNegativeSCEV(Step));</div></div><br class=""></div><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">
If we can only handle counting up here, lets be more clear about that.<br class=""></blockquote><div class=""><br class=""></div><div class="">There is only one case of counting down that we can handle here given the ispowerof2 constraint and that is when step is the maximally negative value (eg, INT_MIN). And in this case Start must be either 0 or equal to step (maximally negative).</div></div></div></div></div></blockquote><div><br class=""></div><div>Exactly, so why complicate the code by handling this case. Why not guard the whole optimization with !CountDown for simplicity?</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">If these cleanups sound reasonable to you I can make the change and add more comments.</div></div></div></div></div></blockquote><div><br class=""></div><div>That would be great! If some assumption is being made at a particular point, like a certain variable must be positive or negative, then there should be either an explicit test, assert, or at least a comment explaining why it’s safe to assume.</div><div><br class=""></div><div>Thanks!</div><div><br class=""></div><div>-Andy</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">Mark</div><div class=""><br class=""></div><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">
<br class="">
<br class="">
<br class="">
<br class="">
<br class="">
<a href="http://reviews.llvm.org/D6546" target="_blank" class="">http://reviews.llvm.org/D6546</a><br class="">
<br class="">
<br class="">
<br class="">
EMAIL PREFERENCES<br class="">
<br class="">
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank" class="">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br class="">
<br class="">
<br class="">
<br class="">
<br class="">
</blockquote></div></div></div>
</div></blockquote></div><br class=""></body></html>