<div dir="ltr">On Thu, Aug 22, 2013 at 9:32 AM, Nadav Rotem <span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank" class="cremed">nrotem@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Chandler,<br>
<br>
Thanks for working on this. Did you measure the difference in compile time of this patch on the LLVM test suite?  The original code avoided calling accumulateConstantOffset when possible because it is somewhat expensive.  I like your pass-wide memoization idea. :)<br>
</blockquote><div><br></div><div>No, because the new version is at least correct. ;] I spot checked and didn't see any huge regressions. Since you're much more familiar with this code, I'll leave fine tuning the performance to you, I merely wanted to correct a miscompile we were seeing in the wild and that blocked our release process.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
><br>
> +  APInt OffsetA(PtrBitWidth, 0), OffsetB(PtrBitWidth, 0);<br>
> +  PtrA = PtrA->stripAndAccumulateInBoundsConstantOffsets(*DL, OffsetA);<br>
> +  PtrB = PtrB->stripAndAccumulateInBoundsConstantOffsets(*DL, OffsetB);<br>
><br>
> -  // Check if PtrB is the base and PtrA is a constant offset.<br>
> -  if (GepA && GepA->getPointerOperand() == PtrB) {<br>
> -    APInt Offset(BW, 0);<br>
> -    if (GepA->accumulateConstantOffset(*DL, Offset))<br>
> -      return Offset.getSExtValue() == -Sz;<br>
> -    return false;<br>
> -  }<br>
> +  APInt OffsetDelta = OffsetB - OffsetA;<br>
> +<br>
> +  // Check if they are based on the same pointer. That makes the offsets<br>
> +  // sufficient.<br>
> +  if (PtrA == PtrB)<br>
> +    return OffsetDelta == Size;<br>
><br>
<br>
</div>What happens if both PtrA and PtrB are zero ?  This means that both OffsetA and OffsetB are garbage. You need to check if at least one of the calls to stripAndAccumulate succeeded.<br></blockquote><div><br></div><div>
Instead, the stripAndAccumulate calls should ensure the offset is preserved when we find a non-constant GEP. That way, a partial walk can still remove some layers of GEPs before we hand it off to SCEV. I've fixed this in r189188 so that we should in the worst case just get zero offsets here. If you have a test case for this, please check it in or send it my way. Good catch by inspection though.</div>
<div><br></div><div>-Chandler</div></div></div></div>