[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

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

14–17

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

6–1

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

1008–1009

I'd make this llvm_unreachable.

[RKSimon]

Thanks David

lib/IR/ConstantFold.cpp

 //===- ConstantFold.cpp - LLVM constant folder ----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements folding of constants for LLVM.  This implements the
 // (internal) ConstantFold.h interface, which is used by the
 // ConstantExpr::get* methods to automatically fold constants when possible.
 //
 // The current constant folding implementation is implemented in two pieces: the
 // pieces that don't need DataLayout, and the pieces that do. This is to avoid
 // a dependence in IR on Target.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ConstantFold.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
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());
+    default:
+      // We don't know how to handle UNDEFs in this instruction.
+      return nullptr;
     }
   }
 
+  // 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: