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

[Michael Zolotukhin]

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:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr17473-v2.patch
Type: application/octet-stream
Size: 4988 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140310/f8c0ec7c/attachment.obj>
-------------- next part --------------


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
>