<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Dec 11, 2014 at 1:26 AM, Andrew Trick <span dir="ltr"><<a href="mailto:atrick@apple.com" target="_blank">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></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>- StepV could be negative but still be a power of two, is this expected?<br></blockquote><div><br></div><div>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><br></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">- Why are we checking GetMinTrailingZeros(getNegativeSCEV(Start)) instead of GetMinTrailingZeros(Distance)?<br></blockquote><div><br></div><div>Isn't this expression always true:  GetMinTrailingZeros(foo) == GetMinTrailingZeros(getNegativeSCEV(foo))  ?  If so, it doesn't matter whether we use Start or 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)">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)">)</span> or GetMinTrailingZeros(Distance) like you suggest.</div><div><br></div><div>Looking at the getUdivExpr:</div><div><br></div><div><div>const SCEV *Exact =</div><div>  getUDivExpr(Distance, CountDown ? getNegativeSCEV(Step) : Step);</div></div><div><br></div><div>This is unnecessarily obfuscated.  It is equivalently:</div><div><br></div><div><div>const SCEV *Exact =</div><div>  getUDivExpr(CountDown ? Start : getNegativeSCEV(Start), CountDown ? getNegativeSCEV(Step) : Step);</div></div><div><br></div><div>Seems like this can be expressed as:</div><div><br></div><div><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)"><div>const SCEV *Exact = getUDivExpr(Start, getNegativeSCEV(Step));</div></div><br></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></blockquote><div><br></div><div>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><br></div><div>If these cleanups sound reasonable to you I can make the change and add more comments.</div><div><br></div><div>Mark</div><div><br></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>
<br>
<br>
<br>
<br>
<a href="http://reviews.llvm.org/D6546" target="_blank">http://reviews.llvm.org/D6546</a><br>
<br>
<br>
<br>
EMAIL PREFERENCES<br>
<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
<br>
<br>
</blockquote></div></div></div>