[DAG] Update foldSelectWithIdentityConstant to use llvm::isNeutralConstant

[RKSimon]

Summary

D133866 added the llvm::isNeutralConstant helper to track neutral/passthrough constants

This patch updates foldSelectWithIdentityConstant to use the helper instead of maintaining its own opcode handling

[pengfei]

It looks to me equals to the old code.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

10753–10754

Are these two comments needed?

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

10753–10754

The NSZ TODO can certainly go - I haven't come across any real world cases where target opcodes would be useful yet, but theoretically they could be, so I might keep that one.

The getBinOpIdentity TODO should probably be a NOTE instead

[goncharov]

as a headsup: I see this change causes failures in Halide https://github.com/halide/Halide/blob/main/test/correctness/div_round_to_zero.cpp on some architectures

[spatel]

In D134966#3833518, @goncharov wrote:

as a headsup: I see this change causes failures in Halide https://github.com/halide/Halide/blob/main/test/correctness/div_round_to_zero.cpp on some architectures

Any chance you can post the IR for that code? My first guess is that we've hoisted a div/rem, and that's illegal for this transform because those are not speculatable ops. Looks like this would only happen with a 512-bit vector on an AVX512VL target if I'm seeing the target hook restriction correctly.

[srj]

Looks like the Halide failure is a SIGFPE on an idiv instruction... we are doing idiv %r15b with edx=0xff, eax=0x8382ff80, r15=0xffffffff... i.e., we are dividing by -1. The result won't fit into 32 bits, so we fail.

[craig.topper]

In D134966#3834797, @srj wrote:

Looks like the Halide failure is a SIGFPE on an idiv instruction... we are doing idiv %r15b with edx=0xff, eax=0x8382ff80, r15=0xffffffff... i.e., we are dividing by -1. The result won't fit into 32 bits, so we fail.

I'm guessing we had (sdiv %x, (select %c, %y, 1)) and turned it into (select %c, (sdiv %x, %y), %x). The original code may have been selecting a safe divisor. The new code is not. What happens if we remove SDIV and UDIV from isNeutralConstant?

[srj]

Update: confirmed that https://reviews.llvm.org/rG17dcbd8165479d5b2d7f827bfcb271b50ee03872 appears to fix the injection in Halide

[LuoYuanke]

It seems this patch cause some performance regression on X86. I created D141782 to fix it.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (N1.getOpcode() != ISD::VSELECT || !N1.hasOneUse())
     return SDValue();
 
   unsigned Opcode = N->getOpcode();
   EVT VT = N->getValueType(0);
   SDValue Cond = N1.getOperand(0);
   SDValue TVal = N1.getOperand(1);
   SDValue FVal = N1.getOperand(2);
 
-  // TODO: The cases should match with IR's ConstantExpr::getBinOpIdentity().
-  // TODO: Target-specific opcodes could be added. Ex: "isCommutativeBinOp()".
-  // TODO: With fast-math (NSZ), allow the opposite-sign form of zero?
-  auto isIdentityConstantForOpcode = [](unsigned Opcode, SDValue V) {
-    if (ConstantFPSDNode *C = isConstOrConstSplatFP(V)) {
-      switch (Opcode) {
-      case ISD::FADD: // X + -0.0 --> X
-        return C->isZero() && C->isNegative();
-      case ISD::FSUB: // X - 0.0 --> X
-        return C->isZero() && !C->isNegative();
-      case ISD::FMUL: // X * 1.0 --> X
-      case ISD::FDIV: // X / 1.0 --> X
-        return C->isExactlyValue(1.0);
-      }
-    }
-    if (ConstantSDNode *C = isConstOrConstSplat(V)) {
-      switch (Opcode) {
-      case ISD::ADD: // X + 0 --> X
-      case ISD::SUB: // X - 0 --> X
-      case ISD::SHL: // X << 0 --> X
-      case ISD::SRA: // X s>> 0 --> X
-      case ISD::SRL: // X u>> 0 --> X
-        return C->isZero();
-      case ISD::MUL: // X * 1 --> X
-        return C->isOne();
-      }
-    }
-    return false;
-  };
-
   // This transform increases uses of N0, so freeze it to be safe.
   // binop N0, (vselect Cond, IDC, FVal) --> vselect Cond, N0, (binop N0, FVal)
-  if (isIdentityConstantForOpcode(Opcode, TVal)) {
+  unsigned OpNo = ShouldCommuteOperands ? 0 : 1;
+  if (isNeutralConstant(Opcode, N->getFlags(), TVal, OpNo)) {
     SDValue F0 = DAG.getFreeze(N0);
     SDValue NewBO = DAG.getNode(Opcode, SDLoc(N), VT, F0, FVal, N->getFlags());
     return DAG.getSelect(SDLoc(N), VT, Cond, F0, NewBO);
   }
   // binop N0, (vselect Cond, TVal, IDC) --> vselect Cond, (binop N0, TVal), N0
-  if (isIdentityConstantForOpcode(Opcode, FVal)) {
+  if (isNeutralConstant(Opcode, N->getFlags(), FVal, OpNo)) {
     SDValue F0 = DAG.getFreeze(N0);
     SDValue NewBO = DAG.getNode(Opcode, SDLoc(N), VT, F0, TVal, N->getFlags());
     return DAG.getSelect(SDLoc(N), VT, Cond, NewBO, F0);
   }
 
   return SDValue();
 }
 

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 
 bool llvm::isMinSignedConstant(SDValue V) {
   ConstantSDNode *Const = dyn_cast<ConstantSDNode>(V);
   return Const != nullptr && Const->isMinSignedValue();
 }
 
 bool llvm::isNeutralConstant(unsigned Opcode, SDNodeFlags Flags, SDValue V,
                              unsigned OperandNo) {
-  if (auto *Const = dyn_cast<ConstantSDNode>(V)) {
+  // NOTE: The cases should match with IR's ConstantExpr::getBinOpIdentity().
+  // TODO: Target-specific opcodes could be added.
+  if (auto *Const = isConstOrConstSplat(V)) {
     switch (Opcode) {
     case ISD::ADD:
     case ISD::OR:
     case ISD::XOR:
     case ISD::UMAX:
       return Const->isZero();
     case ISD::MUL:
       return Const->isOne();
     case ISD::AND:
     case ISD::UMIN:
       return Const->isAllOnes();
     case ISD::SMAX:
       return Const->isMinSignedValue();
     case ISD::SMIN:
       return Const->isMaxSignedValue();
     case ISD::SUB:
     case ISD::SHL:
     case ISD::SRA:
     case ISD::SRL:
       return OperandNo == 1 && Const->isZero();
     case ISD::UDIV:
     case ISD::SDIV:
       return OperandNo == 1 && Const->isOne();
     }
-  } else if (auto *ConstFP = dyn_cast<ConstantFPSDNode>(V)) {
+  } else if (auto *ConstFP = isConstOrConstSplatFP(V)) {
     switch (Opcode) {
     case ISD::FADD:
       return ConstFP->isZero() &&
              (Flags.hasNoSignedZeros() || ConstFP->isNegative());
     case ISD::FSUB:
       return OperandNo == 1 && ConstFP->isZero() &&
              (Flags.hasNoSignedZeros() || !ConstFP->isNegative());
     case ISD::FMUL: