[X86] Don't transform atomic-load-add into an inc/dec when inc/dec is slow

[morisset]

Summary

I had to move code around and turn a static function into a method to get
access to Subtarget, but the only "real" change is the extra check for
!Subtarget->slowIncDec().

[jfb]

lib/Target/X86/X86ISelDAGToDAG.cpp

1711

Can you keep this static and pass in the Subtarget?

[morisset]

lib/Target/X86/X86ISelDAGToDAG.cpp

1711

I could easily do that. I don't see how exactly it would be better, could you explain it to me ?

[jfb]

lib/Target/X86/X86ISelDAGToDAG.cpp

1711

It makes it easier to understand the side-effects that the function can have: it can't access or modify any of the class' internals. It makes it harder to inadvertently add things like this to the function: you have to pass them in as parameters.

[morisset]

Follow jfb suggestion.

[jfb]

lgtm

[morisset]

r219357

lib/Target/X86/X86ISelDAGToDAG.cpp

void dump() {
       else
         dbgs() << "nul";
       dbgs() << " JT" << JT << " Align" << Align << '\n';
     }
 #endif
   };
 }
 
+/// Atomic opcode table
+///
+enum AtomicOpc {
+  ADD,
+  SUB,
+  INC,
+  DEC,
+  OR,
+  AND,
+  XOR,
+  AtomicOpcEnd
+};
+
+enum AtomicSz {
+  ConstantI8,
+  I8,
+  SextConstantI16,
+  ConstantI16,
+  I16,
+  SextConstantI32,
+  ConstantI32,
+  I32,
+  SextConstantI64,
+  ConstantI64,
+  I64,
+  AtomicSzEnd
+};
+
+static const uint16_t AtomicOpcTbl[AtomicOpcEnd][AtomicSzEnd] = {
+  {
+    X86::LOCK_ADD8mi,
+    X86::LOCK_ADD8mr,
+    X86::LOCK_ADD16mi8,
+    X86::LOCK_ADD16mi,
+    X86::LOCK_ADD16mr,
+    X86::LOCK_ADD32mi8,
+    X86::LOCK_ADD32mi,
+    X86::LOCK_ADD32mr,
+    X86::LOCK_ADD64mi8,
+    X86::LOCK_ADD64mi32,
+    X86::LOCK_ADD64mr,
+  },
+  {
+    X86::LOCK_SUB8mi,
+    X86::LOCK_SUB8mr,
+    X86::LOCK_SUB16mi8,
+    X86::LOCK_SUB16mi,
+    X86::LOCK_SUB16mr,
+    X86::LOCK_SUB32mi8,
+    X86::LOCK_SUB32mi,
+    X86::LOCK_SUB32mr,
+    X86::LOCK_SUB64mi8,
+    X86::LOCK_SUB64mi32,
+    X86::LOCK_SUB64mr,
+  },
+  {
+    0,
+    X86::LOCK_INC8m,
+    0,
+    0,
+    X86::LOCK_INC16m,
+    0,
+    0,
+    X86::LOCK_INC32m,
+    0,
+    0,
+    X86::LOCK_INC64m,
+  },
+  {
+    0,
+    X86::LOCK_DEC8m,
+    0,
+    0,
+    X86::LOCK_DEC16m,
+    0,
+    0,
+    X86::LOCK_DEC32m,
+    0,
+    0,
+    X86::LOCK_DEC64m,
+  },
+  {
+    X86::LOCK_OR8mi,
+    X86::LOCK_OR8mr,
+    X86::LOCK_OR16mi8,
+    X86::LOCK_OR16mi,
+    X86::LOCK_OR16mr,
+    X86::LOCK_OR32mi8,
+    X86::LOCK_OR32mi,
+    X86::LOCK_OR32mr,
+    X86::LOCK_OR64mi8,
+    X86::LOCK_OR64mi32,
+    X86::LOCK_OR64mr,
+  },
+  {
+    X86::LOCK_AND8mi,
+    X86::LOCK_AND8mr,
+    X86::LOCK_AND16mi8,
+    X86::LOCK_AND16mi,
+    X86::LOCK_AND16mr,
+    X86::LOCK_AND32mi8,
+    X86::LOCK_AND32mi,
+    X86::LOCK_AND32mr,
+    X86::LOCK_AND64mi8,
+    X86::LOCK_AND64mi32,
+    X86::LOCK_AND64mr,
+  },
+  {
+    X86::LOCK_XOR8mi,
+    X86::LOCK_XOR8mr,
+    X86::LOCK_XOR16mi8,
+    X86::LOCK_XOR16mi,
+    X86::LOCK_XOR16mr,
+    X86::LOCK_XOR32mi8,
+    X86::LOCK_XOR32mi,
+    X86::LOCK_XOR32mr,
+    X86::LOCK_XOR64mi8,
+    X86::LOCK_XOR64mi32,
+    X86::LOCK_XOR64mr,
+  }
+};
+
 namespace {
   //===--------------------------------------------------------------------===//
   /// ISel - X86 specific code to select X86 machine instructions for
   /// SelectionDAG operations.
   ///
   class X86DAGToDAGISel final : public SelectionDAGISel {
     /// Subtarget - Keep a pointer to the X86Subtarget around so that we can
     /// make the right decision when generating code for different targets.
bool SelectTLSADDRAddr(SDValue N, SDValue &Base,
                            SDValue &Scale, SDValue &Index, SDValue &Disp,
                            SDValue &Segment);
     bool SelectScalarSSELoad(SDNode *Root, SDValue N,
                              SDValue &Base, SDValue &Scale,
                              SDValue &Index, SDValue &Disp,
                              SDValue &Segment,
                              SDValue &NodeWithChain);
 
+    SDValue getAtomicLoadArithTargetConstant(SelectionDAG *CurDAG, SDLoc dl,
+                                 enum AtomicOpc &Op, MVT NVT, SDValue Val);
+
     bool TryFoldLoad(SDNode *P, SDValue N,
                      SDValue &Base, SDValue &Scale,
                      SDValue &Index, SDValue &Disp,
                      SDValue &Segment);
 
     /// SelectInlineAsmMemoryOperand - Implement addressing mode selection for
     /// inline asm expressions.
     bool SelectInlineAsmMemoryOperand(const SDValue &Op,

 /// the global base register. Output instructions required to
 /// initialize the global base register, if necessary.
 ///
 SDNode *X86DAGToDAGISel::getGlobalBaseReg() {
   unsigned GlobalBaseReg = getInstrInfo()->getGlobalBaseReg(MF);
   return CurDAG->getRegister(GlobalBaseReg, TLI->getPointerTy()).getNode();
 }
 
-/// Atomic opcode table
-///
-enum AtomicOpc {
-  ADD,
-  SUB,
-  INC,
-  DEC,
-  OR,
-  AND,
-  XOR,
-  AtomicOpcEnd
-};
-
-enum AtomicSz {
-  ConstantI8,
-  I8,
-  SextConstantI16,
-  ConstantI16,
-  I16,
-  SextConstantI32,
-  ConstantI32,
-  I32,
-  SextConstantI64,
-  ConstantI64,
-  I64,
-  AtomicSzEnd
-};
-
-static const uint16_t AtomicOpcTbl[AtomicOpcEnd][AtomicSzEnd] = {
-  {
-    X86::LOCK_ADD8mi,
-    X86::LOCK_ADD8mr,
-    X86::LOCK_ADD16mi8,
-    X86::LOCK_ADD16mi,
-    X86::LOCK_ADD16mr,
-    X86::LOCK_ADD32mi8,
-    X86::LOCK_ADD32mi,
-    X86::LOCK_ADD32mr,
-    X86::LOCK_ADD64mi8,
-    X86::LOCK_ADD64mi32,
-    X86::LOCK_ADD64mr,
-  },
-  {
-    X86::LOCK_SUB8mi,
-    X86::LOCK_SUB8mr,
-    X86::LOCK_SUB16mi8,
-    X86::LOCK_SUB16mi,
-    X86::LOCK_SUB16mr,
-    X86::LOCK_SUB32mi8,
-    X86::LOCK_SUB32mi,
-    X86::LOCK_SUB32mr,
-    X86::LOCK_SUB64mi8,
-    X86::LOCK_SUB64mi32,
-    X86::LOCK_SUB64mr,
-  },
-  {
-    0,
-    X86::LOCK_INC8m,
-    0,
-    0,
-    X86::LOCK_INC16m,
-    0,
-    0,
-    X86::LOCK_INC32m,
-    0,
-    0,
-    X86::LOCK_INC64m,
-  },
-  {
-    0,
-    X86::LOCK_DEC8m,
-    0,
-    0,
-    X86::LOCK_DEC16m,
-    0,
-    0,
-    X86::LOCK_DEC32m,
-    0,
-    0,
-    X86::LOCK_DEC64m,
-  },
-  {
-    X86::LOCK_OR8mi,
-    X86::LOCK_OR8mr,
-    X86::LOCK_OR16mi8,
-    X86::LOCK_OR16mi,
-    X86::LOCK_OR16mr,
-    X86::LOCK_OR32mi8,
-    X86::LOCK_OR32mi,
-    X86::LOCK_OR32mr,
-    X86::LOCK_OR64mi8,
-    X86::LOCK_OR64mi32,
-    X86::LOCK_OR64mr,
-  },
-  {
-    X86::LOCK_AND8mi,
-    X86::LOCK_AND8mr,
-    X86::LOCK_AND16mi8,
-    X86::LOCK_AND16mi,
-    X86::LOCK_AND16mr,
-    X86::LOCK_AND32mi8,
-    X86::LOCK_AND32mi,
-    X86::LOCK_AND32mr,
-    X86::LOCK_AND64mi8,
-    X86::LOCK_AND64mi32,
-    X86::LOCK_AND64mr,
-  },
-  {
-    X86::LOCK_XOR8mi,
-    X86::LOCK_XOR8mr,
-    X86::LOCK_XOR16mi8,
-    X86::LOCK_XOR16mi,
-    X86::LOCK_XOR16mr,
-    X86::LOCK_XOR32mi8,
-    X86::LOCK_XOR32mi,
-    X86::LOCK_XOR32mr,
-    X86::LOCK_XOR64mi8,
-    X86::LOCK_XOR64mi32,
-    X86::LOCK_XOR64mr,
-  }
-};
-
 // Return the target constant operand for atomic-load-op and do simple
 // translations, such as from atomic-load-add to lock-sub. The return value is
 // one of the following 3 cases:
 // + target-constant, the operand could be supported as a target constant.
 // + empty, the operand is not needed any more with the new op selected.
 // + non-empty, otherwise.
-static SDValue getAtomicLoadArithTargetConstant(SelectionDAG *CurDAG,
-                                                SDLoc dl,
-                                                enum AtomicOpc &Op, MVT NVT,
-                                                SDValue Val) {
+SDValue X86DAGToDAGISel::
+getAtomicLoadArithTargetConstant(SelectionDAG *CurDAG, SDLoc dl,
+                                 enum AtomicOpc &Op, MVT NVT, SDValue Val) {
   if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Val)) {
     int64_t CNVal = CN->getSExtValue();
     // Quit if not 32-bit imm.
     if ((int32_t)CNVal != CNVal)
       return Val;
     // Quit if INT32_MIN: it would be negated as it is negative and overflow,
     // producing an immediate that does not fit in the 32 bits available for
     // an immediate operand to sub. However, it still fits in 32 bits for the
     // add (since it is not negated) so we can return target-constant.
     if (CNVal == INT32_MIN)
       return CurDAG->getTargetConstant(CNVal, NVT);
     // For atomic-load-add, we could do some optimizations.
     if (Op == ADD) {
       // Translate to INC/DEC if ADD by 1 or -1.
-      if ((CNVal == 1) || (CNVal == -1)) {
+      if (((CNVal == 1) || (CNVal == -1)) && !Subtarget->slowIncDec()) {
         Op = (CNVal == 1) ? INC : DEC;
         // No more constant operand after being translated into INC/DEC.
         return SDValue();
       }
       // Translate to SUB if ADD by negative value.
       if (CNVal < 0) {
         Op = SUB;
         CNVal = -CNVal;

test/CodeGen/X86/atomic_add.ll

 ; RUN: llc < %s -march=x86-64 -verify-machineinstrs | FileCheck %s
+; RUN: llc < %s -march=x86-64 -mattr=slow-incdec -verify-machineinstrs | FileCheck %s --check-prefix SLOW_INC
 
 ; rdar://7103704
 
 define void @sub1(i32* nocapture %p, i32 %v) nounwind ssp {
 entry:
 ; CHECK-LABEL: sub1:
 ; CHECK: subl
   %0 = atomicrmw sub i32* %p, i32 %v monotonic
   ret void
 }
 
 define void @inc4(i64* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: inc4:
 ; CHECK: incq
+; SLOW_INC-LABEL: inc4:
+; SLOW_INC-NOT: incq
   %0 = atomicrmw add i64* %p, i64 1 monotonic
   ret void
 }
 
 define void @add8(i64* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: add8:
 ; CHECK: addq $2
; CHECK: addq
   %1 = atomicrmw add i64* %p, i64 %0 monotonic
   ret void
 }
 
 define void @inc3(i8* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: inc3:
 ; CHECK: incb
+; SLOW_INC-LABEL: inc3:
+; SLOW_INC-NOT: incb
   %0 = atomicrmw add i8* %p, i8 1 monotonic
   ret void
 }
 
 define void @add7(i8* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: add7:
 ; CHECK: addb $2
; CHECK: addb
   %1 = atomicrmw add i8* %p, i8 %0 monotonic
   ret void
 }
 
 define void @inc2(i16* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: inc2:
 ; CHECK: incw
+; SLOW_INC-LABEL: inc2:
+; SLOW_INC-NOT: incw
   %0 = atomicrmw add i16* %p, i16 1 monotonic
   ret void
 }
 
 define void @add6(i16* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: add6:
 ; CHECK: addw $2
; CHECK: addw
   %1 = atomicrmw add i16* %p, i16 %0 monotonic
   ret void
 }
 
 define void @inc1(i32* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: inc1:
 ; CHECK: incl
+; SLOW_INC-LABEL: inc1:
+; SLOW_INC-NOT: incl
   %0 = atomicrmw add i32* %p, i32 1 monotonic
   ret void
 }
 
 define void @add5(i32* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: add5:
 ; CHECK: addl $2
   %0 = atomicrmw add i32* %p, i32 2 monotonic
   ret void
 }
 
 define void @add1(i32* nocapture %p, i32 %v) nounwind ssp {
 entry:
 ; CHECK-LABEL: add1:
 ; CHECK: addl
   %0 = atomicrmw add i32* %p, i32 %v monotonic
   ret void
 }
 
 define void @dec4(i64* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: dec4:
 ; CHECK: decq
+; SLOW_INC-LABEL: dec4:
+; SLOW_INC-NOT: decq
   %0 = atomicrmw sub i64* %p, i64 1 monotonic
   ret void
 }
 
 define void @sub8(i64* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: sub8:
 ; CHECK: subq $2
; CHECK: subq
   %1 = atomicrmw sub i64* %p, i64 %0 monotonic
   ret void
 }
 
 define void @dec3(i8* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: dec3:
 ; CHECK: decb
+; SLOW_INC-LABEL: dec3:
+; SLOW_INC-NOT: decb
   %0 = atomicrmw sub i8* %p, i8 1 monotonic
   ret void
 }
 
 define void @sub7(i8* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: sub7:
 ; CHECK: subb $2
; CHECK: subb
   %1 = atomicrmw sub i8* %p, i8 %0 monotonic
   ret void
 }
 
 define void @dec2(i16* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: dec2:
 ; CHECK: decw
+; SLOW_INC-LABEL: dec2:
+; SLOW_INC-NOT: decw
   %0 = atomicrmw sub i16* %p, i16 1 monotonic
   ret void
 }
 
 define void @sub6(i16* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: sub6:
 ; CHECK: subw $2
; CHECK: subw
   %1 = atomicrmw sub i16* %p, i16 %0 monotonic
   ret void
 }
 
 define void @dec1(i32* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: dec1:
 ; CHECK: decl
+; SLOW_INC-LABEL: dec1:
+; SLOW_INC-NOT: decl
   %0 = atomicrmw sub i32* %p, i32 1 monotonic
   ret void
 }
 
 define void @sub5(i32* nocapture %p) nounwind ssp {
 entry:
 ; CHECK-LABEL: sub5:
 ; CHECK: subl $2
   %0 = atomicrmw sub i32* %p, i32 2 monotonic
   ret void
 }