[llvm] r228656 - [x86] Fix PR22524: the DAG combiner was incorrectly handling illegal

[Hans Wennborg]

Merged in r228787.

Cheers,
Hans

On Tue, Feb 10, 2015 at 6:08 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> Having thought about this, I think this is safe for 3.6. Thanks Hans.
>
> On Tue, Feb 10, 2015 at 5:21 PM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> On Mon, Feb 9, 2015 at 7:00 PM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>> > Thanks for the quick fix!
>> >
>> >> On 2015-Feb-09, at 18:25, Chandler Carruth <chandlerc at gmail.com> wrote:
>> >>
>> >> Author: chandlerc
>> >> Date: Mon Feb  9 20:25:56 2015
>> >> New Revision: 228656
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=228656&view=rev
>> >> Log:
>> >> [x86] Fix PR22524: the DAG combiner was incorrectly handling illegal
>> >> nodes when folding bitcasts of constants.
>> >>
>> >> We can't fold things and then check after-the-fact whether it was
>> >> legal.
>> >> Once we have formed the DAG node, arbitrary other nodes may have been
>> >> collapsed to it. There is no easy way to go back. Instead, we need to
>> >> test for the specific folding cases we're interested in and ensure
>> >> those
>> >> are legal first.
>> >>
>> >> This could in theory make this less powerful for bitcasting from an
>> >> integer to some vector type, but AFAICT, that can't actually happen in
>> >> the SDAG so its fine. Now, we *only* whitelist specific int->fp and
>> >> fp->int bitcasts for post-legalization folding. I've added the test
>> >> case
>> >> from the PR.
>> >>
>> >> (Also as a note, this does not appear to be in 3.6, no backport needed)
>> >
>> > @Hans: I think Chandler was relying on my bisection from the PR here,
>> > but
>> > I had only bisected the opt+llc.ll case.  (I first tried to bisect the
>> > llc.ll case, but couldn't find a passing revision.)
>> >
>> > I've just checked the llc.ll case on 3.6 (well, slightly old r228165)
>> > and
>> > it *does* repro, and cherry-picking this commit fixes it.
>> >
>> > Probably not *urgent*, since -instcombine isn't exposing it, but still
>> > seems like a good fix.
>>
>> I'm okay with merging this if Chandler thinks the patch is safe.
>>
>>  - Hans
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>