miscompile of vector double-negation + select by InstCombine

[regehr]

https://alive2.llvm.org/ce/z/N1RVAr

this:

define <4 x i32> @f(<4 x i32> %0) {
  %2 = sub <4 x i32> zeroinitializer, %0
  %3 = select <4 x i1> <i1 false, i1 true, i1 poison, i1 poison>, <4 x i32> %2, <4 x i32> %0
  %4 = sub <4 x i32> zeroinitializer, %3
  ret <4 x i32> %4
}

is being optimized to:

define <4 x i32> @f(<4 x i32> %0) {
  ret <4 x i32> %0
}

here's Alive's work explaining why this is wrong:

ERROR: Value mismatch

Example:
<4 x i32> %#0 = < #x00000001 (1), poison, poison, poison >

Source:
<4 x i32> %#2 = < #xffffffff (4294967295, -1), poison, poison, poison >
<4 x i32> %#3 = < #x00000001 (1), poison, poison, poison >
<4 x i32> %#4 = < #xffffffff (4294967295, -1), poison, poison, poison >

Target:
Source value: < #xffffffff (4294967295, -1), poison, poison, poison >
Target value: < #x00000001 (1), poison, poison, poison >

cc @nunoplopes @Hatsunespica

[topperc]

I think the problem is this code in Negator::visitImpl.

  case Instruction::Select: {
    if (isKnownNegation(I->getOperand(1), I->getOperand(2))) {                   
      // Of one hand of select is known to be negation of another hand,          
      // just swap the hands around.                                             
      auto *NewSelect = cast<SelectInst>(I->clone());                            
      // Just swap the operands of the select.                                   
      NewSelect->swapValues();                                                   
      // Don't swap prof metadata, we didn't change the branch behavior.         
      NewSelect->setName(I->getName() + ".neg");                                 
      Builder.Insert(NewSelect);                                                 
      return NewSelect;                                                          
    }   

The IR at the time of that code running was

%2 = sub <4 x i32> <i32 poison, i32 0, i32 0, i32 0>, %0
%3 = select <4 x i1> <i1 false, i1 true, i1 poison, i1 poison>, <4 x i32> %2, <4 x i32> %0
%4 = sub <4 x i32> zeroinitializer, %3

So operand element 0 of %2 is poison. This occurred because element 0 isn't selected so SimplifiedDemandedVectorElts replaced with poison.

Once Negator swaps the operands, now element 1 of %2 is never selected so we end up with sub <4 x i32> poison, %0

It doesn't look like swapping the operands of the select is poison safe.

CC @nikic

[nikic]

Yeah, that analysis sounds right to me. With that in mind, a more direct test case would be:

define <2 x i32> @f(<2 x i1> %c, <2 x i32> %x) {
  %neg = sub <2 x i32> <i32 0, i32 poison>, %x
  %sel = select <2 x i1> %c, <2 x i32> %neg, <2 x i32> %x
  %neg2 = sub <2 x i32> zeroinitializer, %sel
  ret <2 x i32> %neg2
}

[regehr]

off topic but do we want an llvm-reduce pass that shrinks vectors? and maybe also shrinks lane sizes? if so I could work on that sort of thing this summer

[XChy]

off topic but do we want an llvm-reduce pass that shrinks vectors? and maybe also shrinks lane sizes? if so I could work on that sort of thing this summer

Sounds great to me. I think some SLPVectorizer testcases that involve 16 or even 32 lanes should be further reduced.