[InstSimplify] Re-enable select ?, undef, X -> X transform when X is provably not poison

[craig.topper]

Summary

Follow up from the transform being removed in D83360. If X is probably not poison, then the transform is safe.

Still plan to remove or adjust the code from ConstantFolding after this. Also need to audit the partial undef vector handling in InstSimplify just below this code.

[craig.topper]

llvm/lib/Analysis/InstructionSimplify.cpp

4124

Should I be passing CxtI and DT here? I based this off the code in SimplifyInsertElementInst, but maybe i should have based it off of SimplifyFreezeInst?

[nlopes]

llvm/lib/Analysis/InstructionSimplify.cpp

4124

Yes, you can; it's correct to do so.
The only issue is that SimplifySelectInst() doesn't seem to receive the select instruction as argument to be passed as CxtI.

[craig.topper]

Use CxtI and DT if they are passed correctly in the SimplifyQuery

[reames]

LGTM

And thank you for doing this.

[brooksmoses]

I'd erroneously made this comment on the revision (where I think nobody will see it) rather than here, so copying it here:

A question about this: We got a couple of dozen MSAN warnings in the Google codebase from the revision that removed these -- I'm guessing that perhaps what happened was that the undef in question was a use-of-uninitialized-value, and this optimization was hiding the use so the MSAN checks didn't trigger. Is re-enabling this going to make those MSAN warnings go away again, re-hiding this undefined behavior?

[nlopes]

In D83440#2143450, @brooksmoses wrote:

A question about this: We got a couple of dozen MSAN warnings in the Google codebase from the revision that removed these -- I'm guessing that perhaps what happened was that the undef in question was a use-of-uninitialized-value, and this optimization was hiding the use so the MSAN checks didn't trigger. Is re-enabling this going to make those MSAN warnings go away again, re-hiding this undefined behavior?

Yes, it may hide some warnings.
But that's life. Optimizations do hide programming errors. This is not the only one for sure. But it's ok as you are checking the code that will run. If the compiler can "fix" bugs automatically, then developers don't need to worry about those. As long as you keep running these checks continuously to track changes in the compiler like this one :)

[brooksmoses]

Replying to my own question, as I was able to test this sooner than I expected: Yes, it looks like the new MSAN warnings remain after this revision. Excellent! I think that proves that this was a useful fix. :)

[brooksmoses]

In D83440#2143461, @nlopes wrote:

Yes, it may hide some warnings.
But that's life. Optimizations do hide programming errors. This is not the only one for sure. But it's ok as you are checking the code that will run. If the compiler can "fix" bugs automatically, then developers don't need to worry about those. As long as you keep running these checks continuously to track changes in the compiler like this one :)

Fair point, and thanks for the perspective! :) Even though I seem to have gotten "lucky" (or unlucky?) with this case, I'm sure that point will be relevant sooner or later.

llvm/lib/Analysis/InstructionSimplify.cpp

static Value *SimplifySelectInst(Value *Cond, Value *TrueVal, Value *FalseVal,
   if (Cond->getType() == TrueVal->getType() &&
       match(TrueVal, m_One()) && match(FalseVal, m_ZeroInt()))
     return Cond;
 
   // select ?, X, X -> X
   if (TrueVal == FalseVal)
     return TrueVal;
 
+  // If the true or false value is undef, we can fold to the other value as
+  // long as the other value isn't poison.
+  // select ?, undef, X -> X
+  if (isa<UndefValue>(TrueVal) && isGuaranteedNotToBeUndefOrPoison(FalseVal))
+    return FalseVal;
+  // select ?, X, undef -> X
+  if (isa<UndefValue>(FalseVal) && isGuaranteedNotToBeUndefOrPoison(TrueVal))
+    return TrueVal;
+
   // Deal with partial undef vector constants: select ?, VecC, VecC' --> VecC''
   Constant *TrueC, *FalseC;
   if (TrueVal->getType()->isVectorTy() && match(TrueVal, m_Constant(TrueC)) &&
       match(FalseVal, m_Constant(FalseC))) {
     unsigned NumElts = cast<VectorType>(TrueC->getType())->getNumElements();
     SmallVector<Constant *, 16> NewC;
     for (unsigned i = 0; i != NumElts; ++i) {
       // Bail out on incomplete vector constants.

llvm/test/Transforms/InstSimplify/select.ll

 ;
   %s = select i1 %cond, <2 x i32> undef, <2 x i32> %x
   ret <2 x i32> %s
 }
 
 ; These can be folded because the other value is guaranteed not to be poison.
 define i32 @false_undef_true_constant(i1 %cond) {
 ; CHECK-LABEL: @false_undef_true_constant(
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[COND:%.*]], i32 10, i32 undef
-; CHECK-NEXT:    ret i32 [[S]]
+; CHECK-NEXT:    ret i32 10
 ;
   %s = select i1 %cond, i32 10, i32 undef
   ret i32 %s
 }
 
 define i32 @true_undef_false_constant(i1 %cond) {
 ; CHECK-LABEL: @true_undef_false_constant(
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[COND:%.*]], i32 undef, i32 20
-; CHECK-NEXT:    ret i32 [[S]]
+; CHECK-NEXT:    ret i32 20
 ;
   %s = select i1 %cond, i32 undef, i32 20
   ret i32 %s
 }
 
 define <2 x i32> @false_undef_true_constant_vec(i1 %cond) {
 ; CHECK-LABEL: @false_undef_true_constant_vec(
 ; CHECK-NEXT:    ret <2 x i32> <i32 42, i32 -42>
;
   %s = select i1 %cond, <2 x i32> undef, <2 x i32> <i32 -42, i32 42>
   ret <2 x i32> %s
 }
 
 ; If one input is undef and the other is freeze, we can fold it to the freeze.
 define i32 @false_undef_true_freeze(i1 %cond, i32 %x) {
 ; CHECK-LABEL: @false_undef_true_freeze(
 ; CHECK-NEXT:    [[XF:%.*]] = freeze i32 [[X:%.*]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[COND:%.*]], i32 [[XF]], i32 undef
-; CHECK-NEXT:    ret i32 [[S]]
+; CHECK-NEXT:    ret i32 [[XF]]
 ;
   %xf = freeze i32 %x
   %s = select i1 %cond, i32 %xf, i32 undef
   ret i32 %s
 }
 
 define i32 @false_undef_false_freeze(i1 %cond, i32 %x) {
 ; CHECK-LABEL: @false_undef_false_freeze(
 ; CHECK-NEXT:    [[XF:%.*]] = freeze i32 [[X:%.*]]
-; CHECK-NEXT:    [[S:%.*]] = select i1 [[COND:%.*]], i32 undef, i32 [[XF]]
-; CHECK-NEXT:    ret i32 [[S]]
+; CHECK-NEXT:    ret i32 [[XF]]
 ;
   %xf = freeze i32 %x
   %s = select i1 %cond, i32 undef, i32 %xf
   ret i32 %s
 }