[InstCombine] Ensure all undef operands are handled before binary instruction constant folding

[RKSimon]

Summary

As noted in PR18355, this patch makes it clear that all cases with undef operands have been handled before further constant folding is attempted.

[majnemer]

lib/IR/ConstantFold.cpp

1001–1003 ↗

(On Diff #51133)

I don't like this because it will silently accept newly added binary operators.
I'd instead prefer if the switch was over the Opcode static_cast'd to the BinaryOps type (perhaps with an assertion that isBinaryOp(Opcode)).

[RKSimon]

Updated so that we assert on unhandled binary instructions with undef operands.

At present we don't handle float binary instructions so I've split these off with a TODO comment instead of asserting - this patch is about clarity not adding the extra handling support.

[majnemer]

lib/IR/ConstantFold.cpp

1008–1011 ↗

(On Diff #51156)

ConstantFoldBinaryInstruction should only be called with a binary op, I'd move this assert to the start of the function.

[RKSimon]

Moved isBinaryOp assert to start of function - with that done we can remove the 'default' option for undef handling and just assert afterward that neither operand is undef, this will pick up any current/future binary instructions that we don't handle undefs for.

[majnemer]

lib/IR/ConstantFold.cpp

923 ↗

(On Diff #51205)

Can we make this switch (static_cast<BinaryOps>(Opcode)).

This would make the switch fully-covered.

[RKSimon]

Converted the switch statement to cast to Instruction::BinaryOps - requires that BinaryOpsEnd is added as a case option.

[majnemer]

LGTM with nits.

lib/IR/ConstantFold.cpp

1010–1011 ↗

(On Diff #51227)

I'd make this llvm_unreachable.

[RKSimon]

Thanks David

llvm/trunk/lib/IR/ConstantFold.cpp

Constant *llvm::ConstantFoldInsertValueInstruction(Constant *Agg,
   if (ArrayType *AT = dyn_cast<ArrayType>(Agg->getType()))
     return ConstantArray::get(AT, Result);
   return ConstantVector::get(Result);
 }
 
 
 Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode,
                                               Constant *C1, Constant *C2) {
+  assert(Instruction::isBinaryOp(Opcode) && "Non-binary instruction detected");
+
   // Handle UndefValue up front.
   if (isa<UndefValue>(C1) || isa<UndefValue>(C2)) {
-    switch (Opcode) {
+    switch (static_cast<Instruction::BinaryOps>(Opcode)) {
     case Instruction::Xor:
       if (isa<UndefValue>(C1) && isa<UndefValue>(C2))
         // Handle undef ^ undef -> 0 special case. This is a common
         // idiom (misuse).
         return Constant::getNullValue(C1->getType());
       // Fallthrough
     case Instruction::Add:
     case Instruction::Sub:
case Instruction::Shl:
       // X << undef -> undef
       if (isa<UndefValue>(C2))
         return C2;
       // undef << 0 -> undef
       if (match(C2, m_Zero()))
         return C1;
       // undef << X -> 0
       return Constant::getNullValue(C1->getType());
+    case Instruction::FAdd:
+    case Instruction::FSub:
+    case Instruction::FMul:
+    case Instruction::FDiv:
+    case Instruction::FRem:
+      // TODO: UNDEF handling for binary float instructions.
+      return nullptr;
+    case Instruction::BinaryOpsEnd:
+      llvm_unreachable("Invalid BinaryOp");
     }
   }
 
+  // At this point neither constant should be an UndefValue.
+  assert(!isa<UndefValue>(C1) && !isa<UndefValue>(C2) &&
+         "Unexpected UndefValue");
+
   // Handle simplifications when the RHS is a constant int.
   if (ConstantInt *CI2 = dyn_cast<ConstantInt>(C2)) {
     switch (Opcode) {
     case Instruction::Add:
       if (CI2->equalsInt(0)) return C1;                         // X + 0 == X
       break;
     case Instruction::Sub:
       if (CI2->equalsInt(0)) return C1;                         // X - 0 == X
case Instruction::AShr:
       break;
     }
   } else if (isa<ConstantInt>(C1)) {
     // If C1 is a ConstantInt and C2 is not, swap the operands.
     if (Instruction::isCommutative(Opcode))
       return ConstantExpr::get(Opcode, C2, C1);
   }
 
-  // At this point we know neither constant is an UndefValue.
   if (ConstantInt *CI1 = dyn_cast<ConstantInt>(C1)) {
     if (ConstantInt *CI2 = dyn_cast<ConstantInt>(C2)) {
       const APInt &C1V = CI1->getValue();
       const APInt &C2V = CI2->getValue();
       switch (Opcode) {
       default:
         break;
       case Instruction::Add: