[InstCombine] Do not fold scalar ops over select with vector condition.

[fhahn]

Summary

If OtherOpT or OtherOpF have scalar types and the condition is a vector,
we would create an invalid select.

[spatel]

LGTM - see inline for some minor bits.

lib/Transforms/InstCombine/InstCombineSelect.cpp

365 ↗

(On Diff #164396)

I found this comment confusing. The select operands themselves are vectors, but the operands of those operands must also be vectors. This can only be a problem with GEPs? If that's correct, let's state that in the comment.

test/Transforms/InstCombine/select-gep.ll

141 ↗

(On Diff #164396)

We could reduce this a bit I think:

define <2 x i64*> @test5(i64* %gep1, i64* %gep2, <2 x i1> %cc) {
  %gep3 = getelementptr i64, i64* %gep1, <2 x i64> undef
  %gep4 = getelementptr i64, i64* %gep2, <2 x i64> undef
  %select = select <2 x i1> %cc, <2 x i64*> %gep3, <2 x i64*> %gep4
  ret <2 x i64*> %select
}

[fhahn]

Adjusted comment and simplified test case.

[fhahn]

lib/Transforms/InstCombine/InstCombineSelect.cpp

365 ↗

(On Diff #164396)

I tried to make it more explicit, what do you think? I *think* it only can happen for getlementptr currently, but there might be other cases ,e.g. if the code around here becomes more powerful)

test/Transforms/InstCombine/select-gep.ll

141 ↗

(On Diff #164396)

Thanks! I also replaced the undef operand.

[spatel]

lib/Transforms/InstCombine/InstCombineSelect.cpp

365 ↗

(On Diff #164396)

Works for me - thanks!

[fhahn]

Thank you very much for having a look so quickly!

llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp

if (TI->getOperand(0) == FI->getOperand(0)) {
     MatchOp  = TI->getOperand(1);
     OtherOpT = TI->getOperand(0);
     OtherOpF = FI->getOperand(1);
     MatchIsOpZero = true;
   } else {
     return nullptr;
   }
 
+  // If the select condition is a vector, the operands of the original select's
+  // operands also must be vectors. This may not be the case for getelementptr
+  // for example.
+  if (SI.getCondition()->getType()->isVectorTy() &&
+      (!OtherOpT->getType()->isVectorTy() ||
+       !OtherOpF->getType()->isVectorTy()))
+    return nullptr;
+
   // If we reach here, they do have operations in common.
   Value *NewSI = Builder.CreateSelect(SI.getCondition(), OtherOpT, OtherOpF,
                                       SI.getName() + ".v", &SI);
   Value *Op0 = MatchIsOpZero ? MatchOp : NewSI;
   Value *Op1 = MatchIsOpZero ? NewSI : MatchOp;
   if (auto *BO = dyn_cast<BinaryOperator>(TI)) {
     BinaryOperator *NewBO = BinaryOperator::Create(BO->getOpcode(), Op0, Op1);
     NewBO->copyIRFlags(TI);

llvm/trunk/test/Transforms/InstCombine/select-gep.ll

 ;
   %gep1 = getelementptr inbounds i32, i32* %p, i64 %x
   %gep2 = getelementptr inbounds i32, i32* %q, i64 %y
   %cmp = icmp ugt i64 %x, %y
   %select = select i1 %cmp, i32* %gep1, i32* %gep2
   ret i32* %select
 }
 
+; We cannot create a select with a vector condition but scalar operands.
+
+define <2 x i64*> @test5(i64* %p1, i64* %p2, <2 x i64> %idx, <2 x i1> %cc) {
+; CHECK-LABEL: @test5(
+; CHECK-NEXT:    [[GEP1:%.*]] = getelementptr i64, i64* %p1, <2 x i64> %idx
+; CHECK-NEXT:    [[GEP2:%.*]] = getelementptr i64, i64* %p2, <2 x i64> %idx
+; CHECK-NEXT:    [[SELECT:%.*]] = select <2 x i1> %cc, <2 x i64*> [[GEP1]], <2 x i64*> [[GEP2]]
+; CHECK-NEXT:    ret <2 x i64*> [[SELECT]]
+;
+  %gep1 = getelementptr i64, i64* %p1, <2 x i64> %idx
+  %gep2 = getelementptr i64, i64* %p2, <2 x i64> %idx
+  %select = select <2 x i1> %cc, <2 x i64*> %gep1, <2 x i64*> %gep2
+  ret <2 x i64*> %select
+}