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

[Erik Verbruggen]

I reverted the patch, together with Reid's follow-up patch.

I'm very interested in suggestions on how to fix this "properly" :-)

-- Erik.

On 28 Mar 2014, at 13:54, Erik Verbruggen <erik.verbruggen at me.com> wrote:

> FWIW: patch with "fix" attached.
> 
> I have to say that I don't really see the ordering problem, so could you enlighten me?
> 
> <0001-InstCombine-do-not-merge-icmp-X-C1-C2-when-the-add-h.patch>
> 
> 
> 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
> 
> _______________________________________________
> 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/99f1e86c/attachment.html>