[X86][CCMP] Fix invalid CCMP emission (llvm#211161)

phoebewang · tru · commit e39f33c9efdb · 2026-07-24T08:33:03.000+02:00
This patch ports AArch64's negation-aware conjunction algorithm to fix invalid CCMP emission when OR nested inside an AND. Example: https://godbolt.org/z/ave7f61hK Before the change, the above case returns 5 rather the 9 when CCMP enabled. Assisted-by: Claude Opus 4.8 (cherry picked from commit 45b3065)
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -25494,52 +25494,102 @@ static SDValue LowerSELECTWithCmpZero(SDValue CmpVal, SDValue LHS, SDValue RHS,
   return SDValue();
 }
 
-// Return true if Val is an integer ISD::SETCC or an AND/OR tree thereof,
-// suitable for lowering to a CCMP chain.
-static bool canEmitConjunctionForCCMP(SDValue Val) {
-  unsigned Opc = Val.getOpcode();
-  if (Opc == ISD::SETCC)
-    return Val.getOperand(0).getSimpleValueType().isInteger();
-  if (Opc == ISD::AND && Val.hasOneUse())
-    return canEmitConjunctionForCCMP(Val.getOperand(0)) &&
-           canEmitConjunctionForCCMP(Val.getOperand(1));
-  // For OR, at least one operand must be a leaf SETCC so the DCF of the right
-  // CCMP is unambiguous.
-  if (Opc == ISD::OR && Val.hasOneUse())
-    return (Val.getOperand(0).getOpcode() == ISD::SETCC ||
-            Val.getOperand(1).getOpcode() == ISD::SETCC) &&
-           canEmitConjunctionForCCMP(Val.getOperand(0)) &&
-           canEmitConjunctionForCCMP(Val.getOperand(1));
+/// Returns true if \p Val is a tree of integer AND/OR/SETCC operations that
+/// can be expressed as a CCMP conjunction. This mirrors AArch64's
+/// canEmitConjunction and tracks the negation bookkeeping required to lower
+/// arbitrarily nested AND/OR trees (e.g. AND(cc, OR(cc, cc))) correctly.
+/// \param CanNegate   Set to true if the whole sub-tree can be negated just by
+///                    inverting the conditions on the SETCC leaves.
+/// \param MustBeFirst Set to true if this sub-tree needs to be negated but
+///                    cannot be negated naturally, so it must be emitted first.
+/// \param WillNegate  True when the result of this sub-expression must be
+///                    negated (i.e. the outer expression is an OR).
+static bool canEmitConjunctionForCCMP(SDValue Val, bool &CanNegate,
+                                      bool &MustBeFirst, bool WillNegate,
+                                      unsigned Depth = 0) {
+  if (!Val.hasOneUse())
+    return false;
+  unsigned Opcode = Val.getOpcode();
+  if (Opcode == ISD::SETCC) {
+    if (!Val.getOperand(0).getSimpleValueType().isInteger())
+      return false;
+    CanNegate = true;
+    MustBeFirst = false;
+    return true;
+  }
+  // Protect against exponential runtime and stack overflow.
+  if (Depth > 6)
+    return false;
+  if (Opcode == ISD::AND || Opcode == ISD::OR) {
+    bool IsOR = Opcode == ISD::OR;
+    SDValue O0 = Val.getOperand(0);
+    SDValue O1 = Val.getOperand(1);
+    bool CanNegateL, MustBeFirstL;
+    if (!canEmitConjunctionForCCMP(O0, CanNegateL, MustBeFirstL, IsOR,
+                                   Depth + 1))
+      return false;
+    bool CanNegateR, MustBeFirstR;
+    if (!canEmitConjunctionForCCMP(O1, CanNegateR, MustBeFirstR, IsOR,
+                                   Depth + 1))
+      return false;
+
+    if (MustBeFirstL && MustBeFirstR)
+      return false;
+
+    if (IsOR) {
+      // For an OR we need to be able to naturally negate at least one side or
+      // we cannot do the transformation at all.
+      if (!CanNegateL && !CanNegateR)
+        return false;
+      // If the OR's result will be negated and both leaves can be negated
+      // naturally, then this sub-tree as a whole negates naturally.
+      CanNegate = WillNegate && CanNegateL && CanNegateR;
+      MustBeFirst = !CanNegate;
+    } else {
+      assert(Opcode == ISD::AND && "Must be OR or AND");
+      // We cannot naturally negate an AND operation.
+      CanNegate = false;
+      MustBeFirst = MustBeFirstL || MustBeFirstR;
+    }
+    return true;
+  }
   return false;
 }
 
-// Recursively emit a CCMP chain for an AND/OR tree of integer SETCCs.
-//   CCOp:      incoming flags value (null for the first/root comparison)
-//   Predicate: condition under which CCOp was produced (COND_INVALID at root)
-//   OutCC:     set to the condition code to test after the whole chain
-// Returns the flags-producing node (SUB or CCMP).
-//
-// AND(cc1, cc2): emit cc1 first; CCMP(cc2) fires when cc1 is true.
-//   SrcCC = cc1,  DCF forces cc2 false when cc1 is false.
-// OR(cc1, cc2):  emit cc1 first; CCMP(cc2) fires when cc1 is false.
-//   SrcCC = ~cc1, DCF forces cc2 true when cc1 is true.
+/// Emit a conjunction or disjunction tree as a CMP followed by a chain of
+/// CCMP operations. Mirrors AArch64's emitConjunctionRec.
+///   CCOp:      incoming flags value (null for the first/root comparison)
+///   Predicate: condition under which CCOp was produced (COND_INVALID at root)
+///   Negate:    true if this sub-tree should be negated by inverting the
+///              conditions on its SETCC leaves
+///   OutCC:     set to the condition code to test after the whole chain
+/// Returns the flags-producing node (CMP/SUB or CCMP).
 static SDValue emitConjunctionForCCMPRec(SDValue Val, X86::CondCode &OutCC,
-                                         SDValue CCOp, X86::CondCode Predicate,
+                                         bool Negate, SDValue CCOp,
+                                         X86::CondCode Predicate,
                                          SelectionDAG &DAG,
                                          const X86Subtarget &Subtarget) {
   SDLoc DL(Val);
 
-  if (Val.getOpcode() == ISD::SETCC) {
+  unsigned Opcode = Val.getOpcode();
+  if (Opcode == ISD::SETCC) {
     SDValue LHS = Val.getOperand(0), RHS = Val.getOperand(1);
     ISD::CondCode CC = cast<CondCodeSDNode>(Val.getOperand(2))->get();
+    if (Negate)
+      CC = ISD::getSetCCInverse(CC, LHS.getValueType());
     X86::CondCode X86CC = TranslateX86CC(CC, DL, /*IsFP=*/false, LHS, RHS, DAG);
     assert(X86CC != X86::COND_INVALID);
     OutCC = X86CC;
 
     SDValue Flags = EmitCmp(LHS, RHS, X86CC, DL, DAG, Subtarget);
+    // Produce a normal comparison if we are first in the chain.
     if (!CCOp)
       return Flags;
 
+    // Otherwise produce a CCMP. The CCMP executes (and updates EFLAGS) only
+    // when Predicate holds; when it is skipped it forces the default condition
+    // flags, which must make OutCC evaluate to false. That default is encoded
+    // from the opposite of X86CC.
     SDNode *FlagsNode = Flags.getNode();
     X86::CondCode DCFCode = X86::GetOppositeBranchCondition(X86CC);
     SDValue CFlags = DAG.getTargetConstant(
@@ -25549,47 +25599,78 @@ static SDValue emitConjunctionForCCMPRec(SDValue Val, X86::CondCode &OutCC,
                        {FlagsNode->getOperand(0), FlagsNode->getOperand(1),
                         CFlags, SrcCC, CCOp});
   }
+  assert(Val.hasOneUse() && "Valid conjunction/disjunction tree");
 
-  bool IsOR = Val.getOpcode() == ISD::OR;
-  SDValue LHS = Val.getOperand(0), RHS = Val.getOperand(1);
-
-  // For OR, the right subtree must be a leaf SETCC so its DCF unambiguously
-  // forces the outcome true when skipped. OR is commutative, so swap if needed.
-  if (IsOR && RHS.getOpcode() != ISD::SETCC)
-    std::swap(LHS, RHS);
-
-  // Emit the left subtree first (provides CCOp for the right subtree's CCMP).
-  X86::CondCode LHSCC;
-  SDValue CmpL =
-      emitConjunctionForCCMPRec(LHS, LHSCC, CCOp, Predicate, DAG, Subtarget);
+  bool IsOR = Opcode == ISD::OR;
 
-  // For AND: right CCMP fires when left is true,  SrcCC = LHSCC.
-  // For OR:  right CCMP fires when left is false, SrcCC = !LHSCC.
-  X86::CondCode NextPred =
-      IsOR ? X86::GetOppositeBranchCondition(LHSCC) : LHSCC;
+  SDValue LHS = Val.getOperand(0);
+  bool CanNegateL, MustBeFirstL;
+  bool ValidL = canEmitConjunctionForCCMP(LHS, CanNegateL, MustBeFirstL, IsOR);
+  assert(ValidL && "Valid conjunction/disjunction tree");
+  (void)ValidL;
 
-  SDValue CmpR =
-      emitConjunctionForCCMPRec(RHS, OutCC, CmpL, NextPred, DAG, Subtarget);
+  SDValue RHS = Val.getOperand(1);
+  bool CanNegateR, MustBeFirstR;
+  bool ValidR = canEmitConjunctionForCCMP(RHS, CanNegateR, MustBeFirstR, IsOR);
+  assert(ValidR && "Valid conjunction/disjunction tree");
+  (void)ValidR;
 
-  // For OR, patch the DCF of the right leaf's CCMP to force OutCC TRUE when
-  // the CCMP is skipped (i.e. when the left condition was already true).
-  if (IsOR && CmpR.getOpcode() == X86ISD::CCMP) {
-    SDValue CFlags = DAG.getTargetConstant(
-        X86::getCCMPCondFlagsFromCondCode(OutCC), DL, MVT::i8);
-    CmpR = DAG.getNode(X86ISD::CCMP, DL, MVT::i32,
-                       {CmpR.getOperand(0), CmpR.getOperand(1), CFlags,
-                        CmpR.getOperand(3), CmpR.getOperand(4)});
+  // Swap the sub-tree that must come first to the right side.
+  if (MustBeFirstL) {
+    assert(!MustBeFirstR && "Valid conjunction/disjunction tree");
+    std::swap(LHS, RHS);
+    std::swap(CanNegateL, CanNegateR);
+    std::swap(MustBeFirstL, MustBeFirstR);
   }
-  return CmpR;
+
+  bool NegateR, NegateAfterR, NegateL, NegateAfterAll;
+  if (IsOR) {
+    // Swap the sub-tree that we can negate naturally to the left.
+    if (!CanNegateL) {
+      assert(CanNegateR && "at least one side must be negatable");
+      assert(!MustBeFirstR && "invalid conjunction/disjunction tree");
+      assert(!Negate);
+      std::swap(LHS, RHS);
+      NegateR = false;
+      NegateAfterR = true;
+    } else {
+      // Negate the left sub-tree if possible, otherwise negate the result.
+      NegateR = CanNegateR;
+      NegateAfterR = !CanNegateR;
+    }
+    NegateL = true;
+    NegateAfterAll = !Negate;
+  } else {
+    assert(Opcode == ISD::AND && "Valid conjunction/disjunction tree");
+    assert(!Negate && "Valid conjunction/disjunction tree");
+    NegateL = false;
+    NegateR = false;
+    NegateAfterR = false;
+    NegateAfterAll = false;
+  }
+
+  // Emit sub-trees. The right sub-tree is emitted first so its flags feed the
+  // left sub-tree's CCMP chain.
+  X86::CondCode RHSCC;
+  SDValue CmpR = emitConjunctionForCCMPRec(RHS, RHSCC, NegateR, CCOp, Predicate,
+                                           DAG, Subtarget);
+  if (NegateAfterR)
+    RHSCC = X86::GetOppositeBranchCondition(RHSCC);
+  SDValue CmpL = emitConjunctionForCCMPRec(LHS, OutCC, NegateL, CmpR, RHSCC,
+                                           DAG, Subtarget);
+  if (NegateAfterAll)
+    OutCC = X86::GetOppositeBranchCondition(OutCC);
+  return CmpL;
 }
 
 static SDValue emitConjunctionForCCMP(SDValue Val, X86::CondCode &OutCC,
                                       SelectionDAG &DAG,
                                       const X86Subtarget &Subtarget) {
-  if (!canEmitConjunctionForCCMP(Val))
+  bool DummyCanNegate, DummyMustBeFirst;
+  if (!canEmitConjunctionForCCMP(Val, DummyCanNegate, DummyMustBeFirst, false))
     return SDValue();
-  return emitConjunctionForCCMPRec(Val, OutCC, SDValue(), X86::COND_INVALID,
-                                   DAG, Subtarget);
+  return emitConjunctionForCCMPRec(Val, OutCC, /*Negate=*/false, SDValue(),
+                                   X86::COND_INVALID, DAG, Subtarget);
 }
 
 SDValue X86TargetLowering::LowerSELECT(SDValue Op, SelectionDAG &DAG) const {
diff --git a/llvm/test/CodeGen/X86/apx/ccmp.ll b/llvm/test/CodeGen/X86/apx/ccmp.ll
@@ -2103,16 +2103,16 @@ define i32 @ccmp_cmov_and(i32 %a, i32 %b, i32 %c, i32 %d) {
 ; CHECK-LABEL: ccmp_cmov_and:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    movl %edi, %eax # encoding: [0x89,0xf8]
-; CHECK-NEXT:    cmpl %ecx, %esi # encoding: [0x39,0xce]
-; CHECK-NEXT:    ccmpnel {dfv=} %edx, %edi # encoding: [0x62,0xf4,0x04,0x05,0x39,0xd7]
-; CHECK-NEXT:    cmovll %esi, %eax # encoding: [0x0f,0x4c,0xc6]
+; CHECK-NEXT:    cmpl %edx, %edi # encoding: [0x39,0xd7]
+; CHECK-NEXT:    ccmpll {dfv=zf} %ecx, %esi # encoding: [0x62,0xf4,0x14,0x0c,0x39,0xce]
+; CHECK-NEXT:    cmovnel %esi, %eax # encoding: [0x0f,0x45,0xc6]
 ; CHECK-NEXT:    retq # encoding: [0xc3]
 ;
 ; NDD-LABEL: ccmp_cmov_and:
 ; NDD:       # %bb.0: # %entry
-; NDD-NEXT:    cmpl %ecx, %esi # encoding: [0x39,0xce]
-; NDD-NEXT:    ccmpnel {dfv=} %edx, %edi # encoding: [0x62,0xf4,0x04,0x05,0x39,0xd7]
-; NDD-NEXT:    cmovll %esi, %edi, %eax # encoding: [0x62,0xf4,0x7c,0x18,0x4c,0xfe]
+; NDD-NEXT:    cmpl %edx, %edi # encoding: [0x39,0xd7]
+; NDD-NEXT:    ccmpll {dfv=zf} %ecx, %esi # encoding: [0x62,0xf4,0x14,0x0c,0x39,0xce]
+; NDD-NEXT:    cmovnel %esi, %edi, %eax # encoding: [0x62,0xf4,0x7c,0x18,0x45,0xfe]
 ; NDD-NEXT:    retq # encoding: [0xc3]
 ;
 ; ZU_COMMON-LABEL: ccmp_cmov_and:
@@ -2136,17 +2136,17 @@ define i32 @ccmp_cmov_and_or(i32 %a, i32 %b, i32 %c, i32 %d) {
 ; CHECK-LABEL: ccmp_cmov_and_or:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    movl %edi, %eax # encoding: [0x89,0xf8]
-; CHECK-NEXT:    cmpl %ecx, %esi # encoding: [0x39,0xce]
-; CHECK-NEXT:    ccmpnel {dfv=} %edx, %edi # encoding: [0x62,0xf4,0x04,0x05,0x39,0xd7]
-; CHECK-NEXT:    ccmpgel {dfv=} %ecx, %edi # encoding: [0x62,0xf4,0x04,0x0d,0x39,0xcf]
+; CHECK-NEXT:    cmpl %edx, %edi # encoding: [0x39,0xd7]
+; CHECK-NEXT:    ccmpll {dfv=zf} %ecx, %esi # encoding: [0x62,0xf4,0x14,0x0c,0x39,0xce]
+; CHECK-NEXT:    ccmpel {dfv=} %ecx, %edi # encoding: [0x62,0xf4,0x04,0x04,0x39,0xcf]
 ; CHECK-NEXT:    cmovgl %esi, %eax # encoding: [0x0f,0x4f,0xc6]
 ; CHECK-NEXT:    retq # encoding: [0xc3]
 ;
 ; NDD-LABEL: ccmp_cmov_and_or:
 ; NDD:       # %bb.0: # %entry
-; NDD-NEXT:    cmpl %ecx, %esi # encoding: [0x39,0xce]
-; NDD-NEXT:    ccmpnel {dfv=} %edx, %edi # encoding: [0x62,0xf4,0x04,0x05,0x39,0xd7]
-; NDD-NEXT:    ccmpgel {dfv=} %ecx, %edi # encoding: [0x62,0xf4,0x04,0x0d,0x39,0xcf]
+; NDD-NEXT:    cmpl %edx, %edi # encoding: [0x39,0xd7]
+; NDD-NEXT:    ccmpll {dfv=zf} %ecx, %esi # encoding: [0x62,0xf4,0x14,0x0c,0x39,0xce]
+; NDD-NEXT:    ccmpel {dfv=} %ecx, %edi # encoding: [0x62,0xf4,0x04,0x04,0x39,0xcf]
 ; NDD-NEXT:    cmovgl %esi, %edi, %eax # encoding: [0x62,0xf4,0x7c,0x18,0x4f,0xfe]
 ; NDD-NEXT:    retq # encoding: [0xc3]
 ;
@@ -2176,17 +2176,17 @@ define i32 @ccmp_cmov_and_or_c(i32 %a, i32 %b, i32 %c, i32 %d) {
 ; CHECK-LABEL: ccmp_cmov_and_or_c:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    movl %edi, %eax # encoding: [0x89,0xf8]
-; CHECK-NEXT:    cmpl %edx, %edi # encoding: [0x39,0xd7]
-; CHECK-NEXT:    ccmpll {dfv=zf} %ecx, %esi # encoding: [0x62,0xf4,0x14,0x0c,0x39,0xce]
-; CHECK-NEXT:    ccmpel {dfv=} %ecx, %edi # encoding: [0x62,0xf4,0x04,0x04,0x39,0xcf]
+; CHECK-NEXT:    cmpl %ecx, %esi # encoding: [0x39,0xce]
+; CHECK-NEXT:    ccmpnel {dfv=} %edx, %edi # encoding: [0x62,0xf4,0x04,0x05,0x39,0xd7]
+; CHECK-NEXT:    ccmpgel {dfv=} %ecx, %edi # encoding: [0x62,0xf4,0x04,0x0d,0x39,0xcf]
 ; CHECK-NEXT:    cmovgl %esi, %eax # encoding: [0x0f,0x4f,0xc6]
 ; CHECK-NEXT:    retq # encoding: [0xc3]
 ;
 ; NDD-LABEL: ccmp_cmov_and_or_c:
 ; NDD:       # %bb.0: # %entry
-; NDD-NEXT:    cmpl %edx, %edi # encoding: [0x39,0xd7]
-; NDD-NEXT:    ccmpll {dfv=zf} %ecx, %esi # encoding: [0x62,0xf4,0x14,0x0c,0x39,0xce]
-; NDD-NEXT:    ccmpel {dfv=} %ecx, %edi # encoding: [0x62,0xf4,0x04,0x04,0x39,0xcf]
+; NDD-NEXT:    cmpl %ecx, %esi # encoding: [0x39,0xce]
+; NDD-NEXT:    ccmpnel {dfv=} %edx, %edi # encoding: [0x62,0xf4,0x04,0x05,0x39,0xd7]
+; NDD-NEXT:    ccmpgel {dfv=} %ecx, %edi # encoding: [0x62,0xf4,0x04,0x0d,0x39,0xcf]
 ; NDD-NEXT:    cmovgl %esi, %edi, %eax # encoding: [0x62,0xf4,0x7c,0x18,0x4f,0xfe]
 ; NDD-NEXT:    retq # encoding: [0xc3]
 ;
@@ -2210,5 +2210,47 @@ entry:
   ret i32 %sel
 }
 
+; (b != d) && ((a < c) || (a > d)): OR nested inside an AND. This shape needs
+; the De Morgan negation bookkeeping in the CCMP conjunction emitter; a naive
+; emitter miscompiles it (the inner OR wrongly forces the result true when the
+; AND's left operand is false).
+define i32 @ccmp_cmov_and_of_or(i32 %a, i32 %b, i32 %c, i32 %d) {
+; CHECK-LABEL: ccmp_cmov_and_of_or:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    movl %edi, %eax # encoding: [0x89,0xf8]
+; CHECK-NEXT:    cmpl %ecx, %edi # encoding: [0x39,0xcf]
+; CHECK-NEXT:    ccmplel {dfv=sf} %edx, %edi # encoding: [0x62,0xf4,0x24,0x0e,0x39,0xd7]
+; CHECK-NEXT:    ccmpll {dfv=zf} %ecx, %esi # encoding: [0x62,0xf4,0x14,0x0c,0x39,0xce]
+; CHECK-NEXT:    cmovnel %esi, %eax # encoding: [0x0f,0x45,0xc6]
+; CHECK-NEXT:    retq # encoding: [0xc3]
+;
+; NDD-LABEL: ccmp_cmov_and_of_or:
+; NDD:       # %bb.0: # %entry
+; NDD-NEXT:    cmpl %ecx, %edi # encoding: [0x39,0xcf]
+; NDD-NEXT:    ccmplel {dfv=sf} %edx, %edi # encoding: [0x62,0xf4,0x24,0x0e,0x39,0xd7]
+; NDD-NEXT:    ccmpll {dfv=zf} %ecx, %esi # encoding: [0x62,0xf4,0x14,0x0c,0x39,0xce]
+; NDD-NEXT:    cmovnel %esi, %edi, %eax # encoding: [0x62,0xf4,0x7c,0x18,0x45,0xfe]
+; NDD-NEXT:    retq # encoding: [0xc3]
+;
+; ZU_COMMON-LABEL: ccmp_cmov_and_of_or:
+; ZU_COMMON:       # %bb.0: # %entry
+; ZU_COMMON-NEXT:    cmpl %ecx, %edi # encoding: [0x39,0xcf]
+; ZU_COMMON-NEXT:    movl %edi, %eax # encoding: [0x89,0xf8]
+; ZU_COMMON-NEXT:    cmovgl %esi, %eax # encoding: [0x0f,0x4f,0xc6]
+; ZU_COMMON-NEXT:    cmpl %edx, %edi # encoding: [0x39,0xd7]
+; ZU_COMMON-NEXT:    cmovll %esi, %eax # encoding: [0x0f,0x4c,0xc6]
+; ZU_COMMON-NEXT:    cmpl %ecx, %esi # encoding: [0x39,0xce]
+; ZU_COMMON-NEXT:    cmovel %edi, %eax # encoding: [0x0f,0x44,0xc7]
+; ZU_COMMON-NEXT:    retq # encoding: [0xc3]
+entry:
+  %cmp1 = icmp ne i32 %b, %d
+  %cmp2 = icmp slt i32 %a, %c
+  %cmp3 = icmp sgt i32 %a, %d
+  %or = or i1 %cmp2, %cmp3
+  %and = and i1 %cmp1, %or
+  %sel = select i1 %and, i32 %b, i32 %a
+  ret i32 %sel
+}
+
 declare dso_local void @foo(...)
 declare {i64, i1} @llvm.ssub.with.overflow.i64(i64, i64) nounwind readnone
