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

[Hans Wennborg via llvm-commits]

On Tue, Aug 18, 2015 at 5:09 PM, Quentin Colombet via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
> On Aug 18, 2015, at 4:31 PM, Quentin Colombet via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>
> Thanks Hal for your responsiveness!
>
> I’ll revert r221876 and add a test case at the same time for PR24468.
>
>
> This is r245394 and r245395.

Thanks! Merged in r245476 and r245478.

Cheers,
Hans


> 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