[PATCH] PR17473: Disallow LSR to perform non-invertible normalization

[Andrew Trick]

On Mar 10, 2014, at 4:31 PM, Michael Zolotukhin <mzolotukhin at apple.com> wrote:

> On Mar 10, 2014, at 3:35 PM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:
> 
>> The code LGTM. Maybe we add a comment like:
>> 
>> +      // PostIncNormalization effectively simplifies the expression under
>> +      // pre-increment assumptions. Those assumptions (no wrapping) might not
>> +      // hold for the post-inc value. Catch such cases by making sure the
>> +      // transformation is invertible.
>> +      if (OriginalISE != ISE) {
>> 
>> 
>> Can you remove the definition of the function “fn3” it is already inlined in your test case so you don’t need the definition?
>> 
>> The test case can be further simplified by removing TBAA tags like ", !tbaa !1” and removing the metadata at the end:
>> 
>> +!llvm.ident = !{!0}
>> +
>> +!0 = metadata !{metadata !"clang version 3.5.0 "}
>> +!1 = metadata !{metadata !2, metadata !2, i64 0}
>> +!2 = metadata !{metadata !"int", metadata !3, i64 0}
>> +!3 = metadata !{metadata !"omnipotent char", metadata !4, i64 0}
>> +!4 = metadata !{metadata !"Simple C/C++ TBAA"}
>> +!5 = metadata !{metadata !3, metadata !3, i64 0}
>> 
> 
> That’s reasonable, thanks!
> Here is a corrected patch:
> 
> <pr17473-v2.patch>

Hi Michael,

As we discussed, once you have finished running the test-suite with a check to find out if any important cases are no longer normalized as expected, then I think you can commit. I know you’ve already verified performance, so this is just an extra sanity check.

Thanks.

-Andy

> 
> Thanks,
> Michael
>> Thanks,
>> Arnold
>> 
>> On Mar 10, 2014, at 2:56 PM, Michael Zolotukhin <mzolotukhin at apple.com> wrote:
>> 
>>> Hi,
>>> 
>>> This is a fix for PR17473. The issue is that LSR performs normalization of detected IV users and sometimes wants to denormalize it back hoping to get the original expression. But normalization isn’t always invertible, and the new expression might be not equivalent to the original one. In the test case, we were losing sign-extension in such transformation. The patch simply adds a check, if the transformation is invertible. 
>>> 
>>> <pr17473.patch>
>>> 
>>> Ok for trunk?
>>> 
>>> Thanks,
>>> Michael

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140310/3fa23bec/attachment.html>