[InstSimplify] Forbid undef folds in expandBinOp

[nikic]

Summary

This is the replacement for D84250 based on D84792. As we recursively fold with the same value twice, we need to disable undef folds, to prevent an undef from being folded to two different values.

Reverting the revert rG00f3579aea6e3d4a4b7464c3db47294f71cef9e4 and using the test case from https://reviews.llvm.org/D83360#2145793, it no longer performs the incorrect fold.

[aqjune]

Does this patch need an update at llvm/test/Transforms/InstSimplify/select.ll as well? (relevant commit: https://github.com/llvm/llvm-project/commit/566a66703f020996d07191323ae8ad6f7ad827b3 )

---

Nevermind, it should be updated after the select undef fold is disabled.

[aqjune]

I think this patch is straightforward. I'll accept this; could anyone have a look as well?

[jdoerfert]

Why is the test not part of this?

[aqjune]

In D85684#2208630, @jdoerfert wrote:

Why is the test not part of this?

Maybe it is because an operation on undef is already folded before expandBinOp is reached. Is it right @nikic? I think having a working test here likely means we found another miscompilation regarding undef and distributivity law.
If select c, undef, x --> x is disabled later, it will leave the undef constant, making the test at select.ll valid.

[nikic]

In D85684#2208668, @aqjune wrote:

In D85684#2208630, @jdoerfert wrote:

Why is the test not part of this?

Maybe it is because an operation on undef is already folded before expandBinOp is reached. Is it right @nikic? I think having a working test here likely means we found another miscompilation regarding undef and distributivity law.
If select c, undef, x --> x is disabled later, it will leave the undef constant, making the test at select.ll valid.

That's right. I only added the test to make sure we don't miscompile it once the select fold is disabled. I don't know of a way to directly test this change right now.

[jdoerfert]

In D85684#2209084, @nikic wrote:

In D85684#2208668, @aqjune wrote:

In D85684#2208630, @jdoerfert wrote:

Why is the test not part of this?

Maybe it is because an operation on undef is already folded before expandBinOp is reached. Is it right @nikic? I think having a working test here likely means we found another miscompilation regarding undef and distributivity law.
If select c, undef, x --> x is disabled later, it will leave the undef constant, making the test at select.ll valid.

That's right. I only added the test to make sure we don't miscompile it once the select fold is disabled. I don't know of a way to directly test this change right now.

That's unfortunate but the change looks harmless enough I guess.

llvm/include/llvm/Analysis/InstructionSimplify.h

SimplifyQuery(const DataLayout &DL, const TargetLibraryInfo *TLI,
                 bool CanUseUndef = true)
       : DL(DL), TLI(TLI), DT(DT), AC(AC), CxtI(CXTI), IIQ(UseInstrInfo),
         CanUseUndef(CanUseUndef) {}
   SimplifyQuery getWithInstruction(Instruction *I) const {
     SimplifyQuery Copy(*this);
     Copy.CxtI = I;
     return Copy;
   }
+  SimplifyQuery getWithoutUndef() const {
+    SimplifyQuery Copy(*this);
+    Copy.CanUseUndef = false;
+    return Copy;
+  }
 };
 
 // NOTE: the explicit multiple argument versions of these functions are
 // deprecated.
 // Please use the SimplifyQuery versions in new code.
 
 /// Given operand for an FNeg, fold the result or return null.
 Value *SimplifyFNegInst(Value *Op, FastMathFlags FMF,

llvm/lib/Analysis/InstructionSimplify.cpp

 /// "(B0 op OtherOp) opex (B1 op OtherOp)".
 static Value *expandBinOp(Instruction::BinaryOps Opcode, Value *V,
                           Value *OtherOp, Instruction::BinaryOps OpcodeToExpand,
                           const SimplifyQuery &Q, unsigned MaxRecurse) {
   auto *B = dyn_cast<BinaryOperator>(V);
   if (!B || B->getOpcode() != OpcodeToExpand)
     return nullptr;
   Value *B0 = B->getOperand(0), *B1 = B->getOperand(1);
-  Value *L = SimplifyBinOp(Opcode, B0, OtherOp, Q, MaxRecurse);
+  Value *L = SimplifyBinOp(Opcode, B0, OtherOp, Q.getWithoutUndef(),
+                           MaxRecurse);
   if (!L)
     return nullptr;
-  Value *R = SimplifyBinOp(Opcode, B1, OtherOp, Q, MaxRecurse);
+  Value *R = SimplifyBinOp(Opcode, B1, OtherOp, Q.getWithoutUndef(),
+                           MaxRecurse);
   if (!R)
     return nullptr;
 
   // Does the expanded pair of binops simplify to the existing binop?
   if ((L == B0 && R == B1) ||
       (Instruction::isCommutative(OpcodeToExpand) && L == B1 && R == B0)) {
     ++NumExpand;
     return B;