[PATCH] D11847: [BasicAA] Fix zext & sext handling

Tue Aug 18 16:31:11 PDT 2015

Thanks Hal for your responsiveness!

I’ll revert r221876 and add a test case at the same time for PR24468.

Q.
> On Aug 18, 2015, at 4:20 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> ----- Original Message -----
>> From: "Quentin Colombet" <qcolombet at apple.com>
>> To: reviews+D11847+public+2636e964e792efad at reviews.llvm.org, "Hal J. Finkel" <hfinkel at anl.gov>
>> Cc: "n j white" <n.j.white at gmail.com>, llvm-commits at lists.llvm.org
>> Sent: Tuesday, August 18, 2015 6:04:44 PM
>> Subject: Re: [PATCH] D11847: [BasicAA] Fix zext & sext handling
>> 
>> 
>> 
>> 
>>> On Aug 18, 2015, at 3:43 PM, hfinkel at anl.gov via llvm-commits
>>> <llvm-commits at lists.llvm.org> wrote:
>>> 
>>> hfinkel added a comment.
>>> 
>>> In
>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11847-23227079&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=Kar_gr4lUTX9iRgFLHyIPCR7VD3VlB3-02h_Q5v9oWk&m=MpOmT90EJ-G0_kGQEMbywvqzTIbeadHteqN7q7h7Mn0&s=Jyz-iD2ioHwmBYyshZdhYpv4WHXfL-BZ8AFI6aG1DQc&e=
>>> , @hans wrote:
>>> 
>>>> In
>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11847-23227064&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=Kar_gr4lUTX9iRgFLHyIPCR7VD3VlB3-02h_Q5v9oWk&m=MpOmT90EJ-G0_kGQEMbywvqzTIbeadHteqN7q7h7Mn0&s=jbagtOjQ81ZE2_2Ad6QWvHAVMFDYkZmkNBTZ5R-HQWw&e=
>>>> , @hfinkel wrote:
>>>> 
>>>>> In
>>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11847-23227037&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=Kar_gr4lUTX9iRgFLHyIPCR7VD3VlB3-02h_Q5v9oWk&m=MpOmT90EJ-G0_kGQEMbywvqzTIbeadHteqN7q7h7Mn0&s=onbkCh-aT1_E8mOtBOsk10wWGw5dtFhQvR9FGrC3LgU&e=
>>>>> , @hans wrote:
>>>>> 
>>>>>> Did this get committed? Is this a fix we need for 3.7?
>>>>> 
>>>>> 
>>>>> From what qcolombet mentioned, this fixes a bug introduced in
>>>>> r221876. That would make it nice to have in 3.7, and I'm fine
>>>>> with it being committed to trunk and making my requested
>>>>> commentary improvements in follow-up. However, this is not
>>>>> purely a bug fix, and furthermore, this fix itself has been
>>>>> committed/reverted before as well. I'm nervous about merging it
>>>>> into the release branch late in the release cycle. It might be
>>>>> best to revert r221876 in the release branch instead. Is there
>>>>> going to be another release candidate at this point before 3.7?
>>>> 
>>>> 
>>>> r221876 also doesn't look like a trivial patch. It's hard to
>>>> follow along with all the reverted reverts here.
>>>> 
>>>> From what I understand, PR23626 that's mentioned here was already
>>>> fixed by reverting something else on 3.7, so maybe we don't need
>>>> this or reverting r221876?
>>>> 
>>>> There have been a number of merges after 3.7-rc2, so there will be
>>>> an rc3, but I'm hoping that will become the final one.
>>> 
>>> 
>>> Okay; I'll make sure this gets committed today.
>> 
>> We need to hold on this fix/improvement.
>> I did some more testing with that patch and it turns out it breaks a
>> lot of tests in the LLVM test suite.
>> 
>> For instance, on x86_64 with Os, try
>> SingleSource/Benchmarks/Shoutout/heapsort.
>> 
>> I think at this point it is best to revert r221876 and have a step
>> back on the whole design of this improvement.
>> 
>> Attached the IR output for heap sort with and without the patch.
>> 
>> What do you think?
> 
> Okay. In that case, let's revert.
> 
> -Hal
> 
>> 
>> Thanks,
>> -Quentin
>> 
>> 
>> 
>>> 
>>> 
>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11847&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=Kar_gr4lUTX9iRgFLHyIPCR7VD3VlB3-02h_Q5v9oWk&m=MpOmT90EJ-G0_kGQEMbywvqzTIbeadHteqN7q7h7Mn0&s=4FRBNH1J1Nrf4m77QozOqM-iJHGyXuqpxhgwluZDjcY&e=
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=Kar_gr4lUTX9iRgFLHyIPCR7VD3VlB3-02h_Q5v9oWk&m=MpOmT90EJ-G0_kGQEMbywvqzTIbeadHteqN7q7h7Mn0&s=SMp__3HeFsLsXLuFxQwIe4BmvifY1hwXW_ejIjnQe8I&e=
>> 
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory

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