[InstCombine] Fix folding of select into phi nodes when incoming element of phi is a vector type

[anna]

Summary

When the select condition operand is a phi node with an incoming value as a constant vector, we are incorrectly folding select into that phi node. This optimization is done in FoldOpIntoPhi.
Without the fix, we would incorrectly choose the true part
of the select to fold into the phi node, even if some elements in the vector are null.

With this fix, we will correctly fold the select into the phi node based on the
select vector operand (see added test cases).

[majnemer]

lib/Transforms/InstCombine/InstructionCombining.cpp

911–912 ↗

(On Diff #92491)

I am surprised, what is InC if not a ConstantAggregateZero in this case?

[anna]

lib/Transforms/InstCombine/InstructionCombining.cpp

911–912 ↗

(On Diff #92491)

InC is a ConstantAggregateZero is all vector elements are zero. In that case, isNullValue will return true and it is correct to fold into FalseVInPred.

However, if we have InC as a vector <0,1,1,0>, isNullValue will return false which is also correct, but we will fold into TrueVInPred which is an incorrect fold.

I think I'll have to update my comment which states always returns false. The problem is the incorrect fold when we have non-zero elements in the vector.

For example, in vec1 test case, without this change, we will incorrectly return ret <4 x i64> zeroinitializer.

[anna]

lib/Transforms/InstCombine/InstructionCombining.cpp

913 ↗

(On Diff #92491)

I will remove the will always return false part in the comment. That is incorrect.
Comment will be changed to:

For vector constants, we cannot use isNullValue to fold into FalseVInPred versus TrueVInPred. When we have individual nonzero elements in the vector, we will still incorrectly fold to `TrueVInPred`.

[anna]

ping?
Currently, we have miscompiles when folding select into phi when phi has incoming non-zero constant vectors (both the added test cases show the problem).

[sanjoy]

lib/Transforms/InstCombine/InstructionCombining.cpp

914 ↗

(On Diff #92491)

How about ConstantArray and ConstantStruct?

[anna]

lib/Transforms/InstCombine/InstructionCombining.cpp

914 ↗

(On Diff #92491)

yes, good point. We are currently miscompiling for those types as well (basically all constant aggregate types : struct, array, vector). I'll update the patch and add test cases for these types.

[anna]

lib/Transforms/InstCombine/InstructionCombining.cpp

914 ↗

(On Diff #92491)

Actually, I take that back. We will not fail for those types. Langref states that the condition for select can only be a i1 or <n x i1>, i.e. a boolean or a vector of booleans. Both struct and arrays are different types from vector types.

So, in effect, this miscompile will only effect vectors that are the select condition operand.

[anna]

Updated the comment. NFC from previous diff.

[sanjoy]

Looks good!

test/Transforms/InstCombine/phi-select-constexpr.ll

1 ↗

(On Diff #92951)

Should we rename this file now?

[sanjoy]

lib/Transforms/InstCombine/InstructionCombining.cpp

915 ↗

(On Diff #92951)

Actually, one more thing -- can we rewrite this as a positive test? That is, instead of checking what Inc cannot be, check that it is only allowed to be a ConstantInt?

(I'm fine doing that cleanup as a follow-on change.)

[anna]

lib/Transforms/InstCombine/InstructionCombining.cpp

915 ↗

(On Diff #92951)

Will do.

test/Transforms/InstCombine/phi-select-constexpr.ll

1 ↗

(On Diff #92951)

Maybe something like phi-select-const?

[sanjoy]

test/Transforms/InstCombine/phi-select-constexpr.ll

1 ↗

(On Diff #92951)

How about phi-select-constant?

[anna]

test/Transforms/InstCombine/phi-select-constexpr.ll

1 ↗

(On Diff #92951)

agreed. doing as an NFC.

[anna]

Both changes are done as follow up.

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

if (SelectInst *SI = dyn_cast<SelectInst>(&I)) {
     for (unsigned i = 0; i != NumPHIValues; ++i) {
       BasicBlock *ThisBB = PN->getIncomingBlock(i);
       Value *TrueVInPred = TrueV->DoPHITranslation(PhiTransBB, ThisBB);
       Value *FalseVInPred = FalseV->DoPHITranslation(PhiTransBB, ThisBB);
       Value *InV = nullptr;
       // Beware of ConstantExpr:  it may eventually evaluate to getNullValue,
       // even if currently isNullValue gives false.
       Constant *InC = dyn_cast<Constant>(PN->getIncomingValue(i));
-      if (InC && !isa<ConstantExpr>(InC))
+      // For vector constants, we cannot use isNullValue to fold into
+      // FalseVInPred versus TrueVInPred. When we have individual nonzero
+      // elements in the vector, we will incorrectly fold InC to
+      // `TrueVInPred`.
+      if (InC && !isa<ConstantExpr>(InC) && !isa<VectorType>(InC->getType()))
         InV = InC->isNullValue() ? FalseVInPred : TrueVInPred;
       else
         InV = Builder->CreateSelect(PN->getIncomingValue(i),
                                     TrueVInPred, FalseVInPred, "phitmp");
       NewPN->addIncoming(InV, ThisBB);
     }
   } else if (CmpInst *CI = dyn_cast<CmpInst>(&I)) {
     Constant *C = cast<Constant>(I.getOperand(1));

llvm/trunk/test/Transforms/InstCombine/phi-select-constexpr.ll

 ; RUN: opt < %s -S -instcombine | FileCheck %s
 @A = extern_weak global i32, align 4
 @B = extern_weak global i32, align 4
 
 define i32 @foo(i1 %which) {
 entry:
   br i1 %which, label %final, label %delay
 
 delay:
   br label %final
 
+; CHECK-LABEL: @foo
 ; CHECK-LABEL: final:
 ; CHECK: phi i32 [ 1, %entry ], [ select (i1 icmp eq (i32* @A, i32* @B), i32 2, i32 1), %delay ]
 final:
   %use2 = phi i1 [ false, %entry ], [ icmp eq (i32* @A, i32* @B), %delay ]
   %value = select i1 %use2, i32 2, i32 1
   ret i32 %value
 }
 
+
+; test folding of select into phi for vectors.
+define <4 x i64> @vec1(i1 %which) {
+entry:
+  br i1 %which, label %final, label %delay
+
+delay:
+ br label %final
+
+final:
+; CHECK-LABEL: @vec1
+; CHECK-LABEL: final:
+; CHECK: %phinode = phi <4 x i64> [ zeroinitializer, %entry ], [ <i64 0, i64 0, i64 126, i64 127>, %delay ]
+; CHECK-NOT: select
+; CHECK: ret <4 x i64> %phinode
+ %phinode =  phi <4 x i1> [ <i1 true, i1 true, i1 true, i1 true>, %entry ], [ <i1 true, i1 true, i1 false, i1 false>, %delay ]
+ %sel = select <4 x i1> %phinode, <4 x i64> zeroinitializer, <4 x i64> <i64 124, i64 125, i64 126, i64 127>
+ ret <4 x i64> %sel
+}
+
+define <4 x i64> @vec2(i1 %which) {
+entry:
+  br i1 %which, label %final, label %delay
+
+delay:
+ br label %final
+
+final:
+; CHECK-LABEL: @vec2
+; CHECK-LABEL: final:
+; CHECK: %phinode = phi <4 x i64> [ <i64 124, i64 125, i64 126, i64 127>, %entry ], [ <i64 0, i64 125, i64 0, i64 127>, %delay ]
+; CHECK-NOT: select
+; CHECK: ret <4 x i64> %phinode
+ %phinode =  phi <4 x i1> [ <i1 false, i1 false, i1 false, i1 false>, %entry ], [ <i1 true, i1 false, i1 true, i1 false>, %delay ]
+ %sel = select <4 x i1> %phinode, <4 x i64> zeroinitializer, <4 x i64> <i64 124, i64 125, i64 126, i64 127>
+ ret <4 x i64> %sel
+}