[X86] Reuse EFLAGS and form LOCKed ops when only user is SETCC.

[ab]

Summary

We only generate LOCKed versions of add/sub when the result is unused. It often happens that the result is used, but only by a comparison. We can optimize those out by reusing EFLAGS, which lets us use the proper instructions, instead of having to fallback to LXADD.

Instead of doing this as an MI peephole (as we do for the other non-LOCKed (really, non-MR) forms), do it in ISel. It becomes quite tricky later.

This also makes it possible to stop expanding and/or/xor if the only user is an icmp. I'm a little uncomfortable with the coupling, but I can send a patch if there are no objections?

Note that this builds on several local patches that I can commit right now if desired: I moved the LOCK isel from DAGToDAG to ISelLowering and tablegen. I added the atomic-eflags-update.ll file and split it out to make it easier to review; I'll squash it when committing.

Also, I locally added a combine to canonicalize (LSUB x, c) to (LADD x, -c), but only if c is 1 (to simplify matching INC/DEC). Any objections to generalizing that to any constant? (we do the same for ISD::SUB).

[jfb]

lib/Target/X86/X86ISelLowering.cpp

25538 ↗

(On Diff #49132)

I find the inverted condition hard to read: IIUC you're trying to continue if the opcode is CMP, or if the opcode is SUB and its result is unused? I know checkBoolTestSetCCCombine does the same, and it's equally confusing IMO :)

25571 ↗

(On Diff #49132)

Implementation for lowerAtomicArithWithLOCK?

[ab]

Flip condition to make it more digestible.

[ab]

Rebase away formatting changes.

[ab]

lib/Target/X86/X86ISelLowering.cpp

25577 ↗

(On Diff #49248)

It was in a local refactoring; uploaded D17659 for you!

25538 ↗

(On Diff #49245)

Fair enough; flipped it and added a comment.

[ab]

Ping?

[jfb]

I like this overall, but I'd be more comfortable if someone with better x86-fu that I also reviewed it.

lib/Target/X86/X86ISelLowering.cpp

25556 ↗

(On Diff #49248)

Note: related to @jyknight's D18201. Probably worth fixing (in a separate patch) once both land.

[ab]

Thanks JF!

+more X86 folks; reviews appreciated.

[qcolombet]

Hi Ahmed,

LGTM.

Cheers,
-Quentin

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

if (TLO.ShrinkDemandedConstant(Cond, DemandedMask) ||
                       TLO.New, N->getOperand(1), N->getOperand(2)));
       return SDValue();
     }
   }
 
   return SDValue();
 }
 
+/// Combine:
+///   (brcond/cmov/setcc .., (cmp (atomic_load_op ..), 0), cc)
+/// to:
+///   (brcond/cmov/setcc .., (LOCKed op ..), cc)
+/// i.e., reusing the EFLAGS produced by the LOCKed instruction.
+/// Note that this is only legal for some op/cc combinations.
+static SDValue combineSetCCAtomicArith(SDValue Cmp, X86::CondCode CC,
+                                       SelectionDAG &DAG) {
+  // This combine only operates on CMP-like nodes.
+  if (!(Cmp.getOpcode() == X86ISD::CMP ||
+        (Cmp.getOpcode() == X86ISD::SUB && !Cmp->hasAnyUseOfValue(0))))
+    return SDValue();
+
+  SDValue LHS = Cmp.getOperand(0);
+  SDValue RHS = Cmp.getOperand(1);
+
+  if (!LHS.hasOneUse())
+    return SDValue();
+
+  // FIXME: We can do this for XOR/OR/AND as well, but only if they survive
+  // AtomicExpand. Currently, we choose to expand them to cmpxchg if they
+  // have any users. Could we relax that to ignore (icmp x,0) users?
+  switch (LHS->getOpcode()) {
+  case ISD::ATOMIC_LOAD_ADD:
+  case ISD::ATOMIC_LOAD_SUB:
+    break;
+  default:
+    return SDValue();
+  }
+
+  auto *C = dyn_cast<ConstantSDNode>(RHS);
+  if (!C || C->getZExtValue() != 0)
+    return SDValue();
+
+  // Don't do this for all condition codes, as OF/CF are cleared by (CMP x,0)
+  // but might be set by arithmetic. Furthermore, we might later select INC/DEC,
+  // which don't modify CF (though CCs using CF should have been optimized out).
+  // SF/ZF are safe as they are set the same way.
+  // Note that in theory, the transformation is also valid for P/NP.
+  if (CC != X86::COND_E && CC != X86::COND_NE && CC != X86::COND_S &&
+      CC != X86::COND_NS)
+    return SDValue();
+
+  SDValue LockOp = lowerAtomicArithWithLOCK(LHS, DAG);
+  DAG.ReplaceAllUsesOfValueWith(LHS.getValue(0),
+                                DAG.getUNDEF(LHS.getValueType()));
+  DAG.ReplaceAllUsesOfValueWith(LHS.getValue(1), LockOp.getValue(1));
+  return LockOp;
+}
+
 // Check whether a boolean test is testing a boolean value generated by
 // X86ISD::SETCC. If so, return the operand of that SETCC and proper condition
 // code.
 //
 // Simplify the following patterns:
 // (Op (CMP (SETCC Cond EFLAGS) 1) EQ) or
 // (Op (CMP (SETCC Cond EFLAGS) 0) NEQ)
 // to (Op EFLAGS Cond)
if (SetCC0.getOpcode() != X86ISD::SETCC ||
     return false;
 
   CC0 = (X86::CondCode)SetCC0->getConstantOperandVal(0);
   CC1 = (X86::CondCode)SetCC1->getConstantOperandVal(0);
   Flags = SetCC0->getOperand(1);
   return true;
 }
 
+/// Optimize an EFLAGS definition used according to the condition code \p CC
+/// into a simpler EFLAGS value, potentially returning a new \p CC and replacing
+/// uses of chain values.
+static SDValue combineSetCCEFLAGS(SDValue EFLAGS, X86::CondCode &CC,
+                                  SelectionDAG &DAG) {
+  if (SDValue R = checkBoolTestSetCCCombine(EFLAGS, CC))
+    return R;
+  return combineSetCCAtomicArith(EFLAGS, CC, DAG);
+}
+
 /// Optimize X86ISD::CMOV [LHS, RHS, CONDCODE (e.g. X86::COND_NE), CONDVAL]
 static SDValue combineCMov(SDNode *N, SelectionDAG &DAG,
                            TargetLowering::DAGCombinerInfo &DCI,
                            const X86Subtarget &Subtarget) {
   SDLoc DL(N);
 
   // If the flag operand isn't dead, don't touch this CMOV.
   if (N->getNumValues() == 2 && !SDValue(N, 1).use_empty())
if (CC == X86::COND_E || CC == X86::COND_NE) {
     case X86ISD::BSR:
     case X86ISD::BSF:
       // If operand of BSR / BSF are proven never zero, then ZF cannot be set.
       if (DAG.isKnownNeverZero(Cond.getOperand(0)))
         return (CC == X86::COND_E) ? FalseOp : TrueOp;
     }
   }
 
-  SDValue Flags;
-
-  Flags = checkBoolTestSetCCCombine(Cond, CC);
-  if (Flags.getNode() &&
-      // Extra check as FCMOV only supports a subset of X86 cond.
-      (FalseOp.getValueType() != MVT::f80 || hasFPCMov(CC))) {
-    SDValue Ops[] = { FalseOp, TrueOp,
-                      DAG.getConstant(CC, DL, MVT::i8), Flags };
+  // Try to simplify the EFLAGS and condition code operands.
+  // We can't always do this as FCMOV only supports a subset of X86 cond.
+  if (SDValue Flags = combineSetCCEFLAGS(Cond, CC, DAG)) {
+    if (FalseOp.getValueType() != MVT::f80 || hasFPCMov(CC)) {
+      SDValue Ops[] = {FalseOp, TrueOp, DAG.getConstant(CC, DL, MVT::i8),
+        Flags};
       return DAG.getNode(X86ISD::CMOV, DL, N->getVTList(), Ops);
     }
+  }
 
   // If this is a select between two integer constants, try to do some
   // optimizations.  Note that the operands are ordered the opposite of SELECT
   // operands.
   if (ConstantSDNode *TrueC = dyn_cast<ConstantSDNode>(TrueOp)) {
     if (ConstantSDNode *FalseC = dyn_cast<ConstantSDNode>(FalseOp)) {
       // Canonicalize the TrueC/FalseC values so that TrueC (the true value) is
       // larger than FalseC (the false value).
static SDValue combineX86SetCC(SDNode *N, SelectionDAG &DAG,
   }
 
   // Materialize "setb reg" as "sbb reg,reg", since it can be extended without
   // a zext and produces an all-ones bit which is more useful than 0/1 in some
   // cases.
   if (CC == X86::COND_B)
     return MaterializeSETB(DL, EFLAGS, DAG, N->getSimpleValueType(0));
 
-  if (SDValue Flags = checkBoolTestSetCCCombine(EFLAGS, CC)) {
+  // Try to simplify the EFLAGS and condition code operands.
+  if (SDValue Flags = combineSetCCEFLAGS(EFLAGS, CC, DAG)) {
     SDValue Cond = DAG.getConstant(CC, DL, MVT::i8);
     return DAG.getNode(X86ISD::SETCC, DL, N->getVTList(), Cond, Flags);
   }
 
   return SDValue();
 }
 
 /// Optimize branch condition evaluation.
 static SDValue combineBrCond(SDNode *N, SelectionDAG &DAG,
                              TargetLowering::DAGCombinerInfo &DCI,
                              const X86Subtarget &Subtarget) {
   SDLoc DL(N);
-  SDValue Chain = N->getOperand(0);
-  SDValue Dest = N->getOperand(1);
   SDValue EFLAGS = N->getOperand(3);
   X86::CondCode CC = X86::CondCode(N->getConstantOperandVal(2));
 
-  if (SDValue Flags = checkBoolTestSetCCCombine(EFLAGS, CC)) {
+  // Try to simplify the EFLAGS and condition code operands.
+  // Make sure to not keep references to operands, as combineSetCCEFLAGS can
+  // RAUW them under us.
+  if (SDValue Flags = combineSetCCEFLAGS(EFLAGS, CC, DAG)) {
     SDValue Cond = DAG.getConstant(CC, DL, MVT::i8);
-    return DAG.getNode(X86ISD::BRCOND, DL, N->getVTList(), Chain, Dest, Cond,
-                       Flags);
+    return DAG.getNode(X86ISD::BRCOND, DL, N->getVTList(), N->getOperand(0),
+                       N->getOperand(1), Cond, Flags);
   }
 
   return SDValue();
 }
 
 static SDValue combineVectorCompareAndMaskUnaryOp(SDNode *N,
                                                   SelectionDAG &DAG) {
   // Take advantage of vector comparisons producing 0 or -1 in each lane to

llvm/trunk/test/CodeGen/X86/atomic-eflags-reuse.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-- | FileCheck %s
 
 define i8 @test_add_1_setcc_ne(i64* %p) #0 {
 ; CHECK-LABEL: test_add_1_setcc_ne:
 ; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    movl $1, %eax
-; CHECK-NEXT:    lock xaddq %rax, (%rdi)
-; CHECK-NEXT:    testq %rax, %rax
+; CHECK-NEXT:    lock incq (%rdi)
 ; CHECK-NEXT:    setne %al
 ; CHECK-NEXT:    retq
 entry:
   %tmp0 = atomicrmw add i64* %p, i64 1 seq_cst
   %tmp1 = icmp ne i64 %tmp0, 0
   %tmp2 = zext i1 %tmp1 to i8
   ret i8 %tmp2
 }
 
 define i8 @test_sub_1_setcc_eq(i64* %p) #0 {
 ; CHECK-LABEL: test_sub_1_setcc_eq:
 ; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    movq $-1, %rax
-; CHECK-NEXT:    lock xaddq %rax, (%rdi)
-; CHECK-NEXT:    testq %rax, %rax
+; CHECK-NEXT:    lock decq (%rdi)
 ; CHECK-NEXT:    sete %al
 ; CHECK-NEXT:    retq
 entry:
   %tmp0 = atomicrmw sub i64* %p, i64 1 seq_cst
   %tmp1 = icmp eq i64 %tmp0, 0
   %tmp2 = zext i1 %tmp1 to i8
   ret i8 %tmp2
 }
entry:
   %tmp1 = icmp slt i64 %tmp0, 0
   %tmp2 = zext i1 %tmp1 to i8
   ret i8 %tmp2
 }
 
 define i8 @test_sub_10_setcc_sge(i64* %p) #0 {
 ; CHECK-LABEL: test_sub_10_setcc_sge:
 ; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    movq $-10, %rax
-; CHECK-NEXT:    lock xaddq %rax, (%rdi)
-; CHECK-NEXT:    testq %rax, %rax
+; CHECK-NEXT:    lock addq $-10, (%rdi)
 ; CHECK-NEXT:    setns %al
 ; CHECK-NEXT:    retq
 entry:
   %tmp0 = atomicrmw sub i64* %p, i64 10 seq_cst
   %tmp1 = icmp sge i64 %tmp0, 0
   %tmp2 = zext i1 %tmp1 to i8
   ret i8 %tmp2
 }
 
 ; Test jcc and cmov
 
 define i32 @test_add_10_brcond_sge(i64* %p, i32 %a0, i32 %a1) #0 {
 ; CHECK-LABEL: test_add_10_brcond_sge:
 ; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    movl $10, %eax
-; CHECK-NEXT:    lock xaddq %rax, (%rdi)
-; CHECK-NEXT:    testq %rax, %rax
+; CHECK-NEXT:    lock addq $10, (%rdi)
 ; CHECK-NEXT:    js .LBB4_2
 ; CHECK-NEXT:  # BB#1: # %t
 ; CHECK-NEXT:    movl %esi, %eax
 ; CHECK-NEXT:    retq
 ; CHECK-NEXT:  .LBB4_2: # %f
 ; CHECK-NEXT:    movl %edx, %eax
 ; CHECK-NEXT:    retq
 entry:
   %tmp0 = atomicrmw add i64* %p, i64 10 seq_cst
   %tmp1 = icmp sge i64 %tmp0, 0
   br i1 %tmp1, label %t, label %f
 t:
   ret i32 %a0
 f:
   ret i32 %a1
 }
 
 define i32 @test_sub_1_cmov_slt(i64* %p, i32 %a0, i32 %a1) #0 {
 ; CHECK-LABEL: test_sub_1_cmov_slt:
 ; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    movq $-1, %rax
-; CHECK-NEXT:    lock xaddq %rax, (%rdi)
-; CHECK-NEXT:    testq %rax, %rax
+; CHECK-NEXT:    lock decq (%rdi)
 ; CHECK-NEXT:    cmovnsl %edx, %esi
 ; CHECK-NEXT:    movl %esi, %eax
 ; CHECK-NEXT:    retq
 entry:
   %tmp0 = atomicrmw sub i64* %p, i64 1 seq_cst
   %tmp1 = icmp slt i64 %tmp0, 0
   %tmp2 = select i1 %tmp1, i32 %a0, i32 %a1
   ret i32 %tmp2