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

[Hal Finkel via llvm-commits]

----- 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