Remove BinaryOpsEnd case from ConstantFoldBinaryInstruction(...)

[cameron.mcinally]

Summary

@spatel noticed this in another proposed patch. The isBinaryOp assert at the beginning of ConstantFoldBinaryInstruction(...) ensures that a BinaryOpsEnd will never reach this switch.

I've confirmed that all current BinaryOp Instructions are covered in this switch, so it should be safe to change this case to the default case.

[craig.topper]

Doesn't using BinaryOpsEnd ensure we'll get a warning if any enum value is missing from the switch in the future? Using a plain default would suppress that.

[cameron.mcinally]

In D61555#1490913, @craig.topper wrote:

Doesn't using BinaryOpsEnd ensure we'll get a warning if any enum value is missing from the switch in the future? Using a plain default would suppress that.

There must be something subtle that I don't understand about this. Wouldn't a default case be even stronger -- it would catch those plus any non-BinaryOp that hits the switch?

At least in *+Asserts mode, we should never see a BinaryOpsEnd in this function. The assert on function entry is:

assert(Instruction::isBinaryOp(Opcode) && "Non-binary instruction detected");

Here is isBinaryOp(...):

static inline bool isBinaryOp(unsigned Opcode) {
  return Opcode >= BinaryOpsBegin && Opcode < BinaryOpsEnd;
}

In *-Asserts mode, all bets are off. But I would think that a default case would be preferred there...

[craig.topper]

The type of the input is an enum type there would be a warning for a "fully covered switch default" if a default was placed in a switch the contained all enum values. The compiler assumes that the input will never take a value that isn't in the enum. You should get this warning if you add default without removing BinaryOpsEnd. There's also a warning that gets emitted when a enum value is missing from a switch. Putting a default in the switch will suppress that latter warning.

[cameron.mcinally]

Oh, a proper warning at compile time. That makes sense. I'm okay with abandoning this patch if Sanjay is okay with it.

[spatel]

In D61555#1490960, @cameron.mcinally wrote:

Oh, a proper warning at compile time. That makes sense. I'm okay with abandoning this patch if Sanjay is okay with it.

That's fine; I wasn't thinking about compile-time warning vs. run-time assert.

[cameron.mcinally]

I'm noticing that lib/Analysis/InstructionSimplify.cpp has similar switches that use the default case instead of UnaryOpsEnd and BinaryOpsEnd. Is it worth taking the time to update those for compile-time warnings?

I haven't confirmed which of the switches are candidates yet, so that will need to be done if we move forward with this plan...

[spatel]

In D61555#1491319, @cameron.mcinally wrote:

I'm noticing that lib/Analysis/InstructionSimplify.cpp has similar switches that use the default case instead of UnaryOpsEnd and BinaryOpsEnd. Is it worth taking the time to update those for compile-time warnings?

It's a nicety, but IMO not worth your time. I'd rather get FP strict/fast IR optimization and codegen improvements. :)

llvm/lib/IR/ConstantFold.cpp

Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1,
                                               Constant *C2) {
   assert(Instruction::isBinaryOp(Opcode) && "Non-binary instruction detected");
 
   // Handle scalar UndefValue. Vectors are always evaluated per element.
   bool HasScalarUndef = !C1->getType()->isVectorTy() &&
                         (isa<UndefValue>(C1) || isa<UndefValue>(C2));
   if (HasScalarUndef) {
     switch (static_cast<Instruction::BinaryOps>(Opcode)) {
+    default:
+      llvm_unreachable("Invalid BinaryOp");
     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());
       LLVM_FALLTHROUGH;
     case Instruction::Add:
     case Instruction::Sub:
case Instruction::FRem:
       // [any flop] C, undef -> NaN
       // [any flop] undef, C -> NaN
       // We could potentially specialize NaN/Inf constants vs. 'normal'
       // constants (possibly differently depending on opcode and operand). This
       // would allow returning undef sometimes. But it is always safe to fold to
       // NaN because we can choose the undef operand as NaN, and any FP opcode
       // with a NaN operand will propagate NaN.
       return ConstantFP::getNaN(C1->getType());
-    case Instruction::BinaryOpsEnd:
-      llvm_unreachable("Invalid BinaryOp");
     }
   }
 
   // Neither constant should be UndefValue, unless these are vector constants.
   assert(!HasScalarUndef && "Unexpected UndefValue");
 
   // Handle simplifications when the RHS is a constant int.
   if (ConstantInt *CI2 = dyn_cast<ConstantInt>(C2)) {