[llvm] r204912 - InstCombine: merge constants in both operands of icmp.

[Erik Verbruggen]

FWIW: patch with "fix" attached.

I have to say that I don't really see the ordering problem, so could you enlighten me?




On 27 Mar 2014, at 22:50, Chandler Carruth <chandlerc at google.com> wrote:

> I would suggest reverting and discussing more.
> 
> I'm not really happy with the one-use hack to "fix" this. I think there should be a better way that more directly addresses the problem of extending the liveness of other SSA values, or essentially the canonicalization phase ordering problem that is here.
> 
> 
> On Thu, Mar 27, 2014 at 1:47 PM, Tobias Grosser <tobias at grosser.es> wrote:
> On 03/27/2014 08:42 PM, Reid Kleckner wrote:
> On Thu, Mar 27, 2014 at 12:30 PM, Benjamin Kramer <benny.kra at gmail.com>wrote:
> 
> 
> On 27.03.2014, at 20:23, Erik Verbruggen <erikjv at me.com> wrote:
> 
> Actually, I'm wondering if it's better to revert my patch (and yours).
> The "nightly" test-suite pointed out a regression. If you have a tight for
> loop, like:
>     for (i = 0; i <= 39; i++)
>        doSomethingHere;
> 
> then before the patch it would be:
>     %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
>     %exitcond = icmp eq i64 %indvars.iv.next, 40
> 
> while with the patch it's:
>     %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
>     %exitcond = icmp eq i64 %indvars.iv, 39
> 
> That keeps the "old" %indvars.iv alive. This causes some performance
> tests to fail, like MultiSource/Benchmarks/MiBench/telecomm-gsm.
> 
> I Cc-ed d0k because he did the review, so what do you guys think?
> 
> A simple fix would be to check if the add has just one use. I was hoping
> that this kind of workaround wouldn't be necessary anymore but it looks
> like we still need it in this case :|
> 
> 
> I'm not really an optimizer person, so whatever works for you guys, revert
> or fix forward.  I'm mostly concerned with the miscompile, and that has a
> test case now.  :)
> 
> Yes, I am getting spammed by the LNT buildbots the whole day. Would you guys mind to either commit a fix now or just revert until the issue has been solved.
> 
> Thank you,
> Tobias
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140328/b10350f0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-InstCombine-do-not-merge-icmp-X-C1-C2-when-the-add-h.patch
Type: application/octet-stream
Size: 2184 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140328/b10350f0/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140328/b10350f0/attachment-0001.html>