[llvm] r221693 - LLVM incorrectly folds xor into select

[Oliver Stannard]

The test case I committed and all of the ones you suggested are not affected
by this patch. I made a last-minute change to the test to make it more
robust, here is the original which is miscompiled without the patch:

  ; Do not fold the xor into the select
  define i32 @t15_5(i32 %p) {
  entry:
  ; ARM: mov     [[REG:r[0-9]+]], #2
  ; ARM: cmp     r0, #8
  ; ARM: movwgt  [[REG:r[0-9]+]], #1
  ; ARM: eor     r0, [[REG:r[0-9]+]], #1

  ; T2: movs    [[REG:r[0-9]+]], #2
  ; T2: cmp     [[REG:r[0-9]+]], #8
  ; T2: it      gt
  ; T2: movgt   [[REG:r[0-9]+]], #1
  ; T2: eor     r0, [[REG:r[0-9]+]], #1
    %cmp = icmp sgt i32 %p, 8
    %a = select i1 %cmp, i32 1, i32 2
    %xor = xor i32 %a, 1
    ret i32 %xor
  }

The problem is that isSetCCEquivalent checks for a SELECT_CC between values
that can be used for true and false, but the transformations that rely on
this assume that the values *are* being used as boolean values. This is not
always the case, on ARM (and other targets that use UndefinedBooleanContent)
every even value is considered to be false, so %a is accepted by
isSetCCEquivalent. I originally thought this only affects the XOR case, but
the optimisations for AND and OR can also cause miscompilations with
carefully chosen constants. I think the safest thing to do here is to modify
isSetCCEquivalent to never consider SELECT_CC when a target uses
UndefinedBooleanContent, and I have a patch up at
http://reviews.llvm.org/D6249 to implement this.

Oliver

> -----Original Message-----
> From: Jonathan Roelofs [mailto:jonathan at codesourcery.com]
> Sent: 11 November 2014 18:27
> To: Tim Northover; Oliver Stannard
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm] r221693 - LLVM incorrectly folds xor into select
> 
> 
> 
> On 11/11/14 11:07 AM, Jonathan Roelofs wrote:
> >
> >
> > On 11/11/14 11:00 AM, Tim Northover wrote:
> >>>
> >>> Would you mind also adding tests for what you expect these two to
> be lowered as:
> >>>
> >>> define i32 @t15_2(i32 %p1, i32 %p2, i32 %p3) {
> >>> entry:
> >>>   %cmp = icmp sgt i32 %p1, 8
> >>>   %a = select i1 %cmp, i32 %p2, i32 %p3
> >>>   %xor = xor i32 %a, -1
> >>>   ret i32 %xor
> >>> }
> >>
> >> Isn't this the test that was already added?
> > No...
> >
> > %xor = xor i32 %a, -1
> >
> > vs
> >
> > %xor = xor i32 %a, 1
> >
> > Sorry I didn't call that out more explicitly.
> 
> One more, which I think the case you should actually be checking, given
> the
> setcc peephole that this patch is about:
> 
> define i32 @t15_4(i32 %p1, i32 %p2, i32 %p3) {
> entry:
>    %cmp = icmp sgt i32 %p1, 8
>    %notcmp = xor i1 %cmp, 1
>    %a = select i1 %notcmp, i32 %p2, i32 %p3
>    ret i32 %a
> }
> 
> Are there cases where the result of a setcc is not an i1?
> 
> 
> Jon
> 
> >
> >
> > Cheers,
> >
> > Jon
> >>
> >> Cheers.
> >>
> >> Tim.
> >>
> >
> 
> --
> Jon Roelofs
> jonathan at codesourcery.com
> CodeSourcery / Mentor Embedded