<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;"><br><div><div>On Mar 10, 2014, at 4:31 PM, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com">mzolotukhin@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Mar 10, 2014, at 3:35 PM, Arnold Schwaighofer <<a href="mailto:aschwaighofer@apple.com">aschwaighofer@apple.com</a>> wrote:<br><br><blockquote type="cite">The code LGTM. Maybe we add a comment like:<br><br>+      // PostIncNormalization effectively simplifies the expression under<br>+      // pre-increment assumptions. Those assumptions (no wrapping) might not<br>+      // hold for the post-inc value. Catch such cases by making sure the<br>+      // transformation is invertible.<br>+      if (OriginalISE != ISE) {<br><br><br>Can you remove the definition of the function “fn3” it is already inlined in your test case so you don’t need the definition?<br><br>The test case can be further simplified by removing TBAA tags like ", !tbaa !1” and removing the metadata at the end:<br><br>+!llvm.ident = !{!0}<br>+<br>+!0 = metadata !{metadata !"clang version 3.5.0 "}<br>+!1 = metadata !{metadata !2, metadata !2, i64 0}<br>+!2 = metadata !{metadata !"int", metadata !3, i64 0}<br>+!3 = metadata !{metadata !"omnipotent char", metadata !4, i64 0}<br>+!4 = metadata !{metadata !"Simple C/C++ TBAA"}<br>+!5 = metadata !{metadata !3, metadata !3, i64 0}<br><br></blockquote><br>That’s reasonable, thanks!<br>Here is a corrected patch:<br><br><span><pr17473-v2.patch></span><br></div></blockquote><div><br></div>Hi Michael,</div><div><br></div><div>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.</div><div><br></div><div>Thanks.</div><div><br></div><div>-Andy</div><div><br></div><div><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>Thanks,<br>Michael<br><blockquote type="cite">Thanks,<br>Arnold<br><br>On Mar 10, 2014, at 2:56 PM, Michael Zolotukhin <<a href="mailto:mzolotukhin@apple.com">mzolotukhin@apple.com</a>> wrote:<br><br><blockquote type="cite">Hi,<br><br>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.<span class="Apple-converted-space"> </span><br><br><pr17473.patch><br><br>Ok for trunk?<br><br>Thanks,<br>Michael</blockquote></blockquote></div></blockquote></div><br></body></html>