Use an offset from TOS for idempotent rmw locked op lowering

[reames]

Summary

This was the portion split off D58632 so that it could follow the redzone API cleanup. Note that I changed the offset preferred from -8 to -64. The difference should be very minor, but I thought it might help address one concern which had been previously raised.

[craig.topper]

lib/Target/X86/X86ISelLowering.cpp

26282 ↗

(On Diff #199288)

I think the usual name here would be either TFI or TFL. The type returned is a derived class of TargetFrameLowering.

[reames]

Address review comment, and update one test I'd missed previously.

[jfb]

One question for Chandler (which can be addressed as a follow up), otherwise LGTM.

test/CodeGen/X86/speculative-load-hardening.ll

1148 ↗

(On Diff #199372)

I only just thought about this: should we stick to mfence when the function has attribute speculative_load_hardening? mfence forces much stronger ordering, and doesn't perform an idempotent load / store from the stack (which in this case brings in a new cacheline). Maybe @chandlerc should chime in.

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

static SDValue emitLockedStackOp(SelectionDAG &DAG,
   // operations issued by the current processor.  As such, the location
   // referenced is not relevant for the ordering properties of the instruction.
   // See: Intel® 64 and IA-32 ArchitecturesSoftware Developer’s Manual,
   // 8.2.3.9  Loads and Stores Are Not Reordered with Locked Instructions 
   // 2) Using an immediate operand appears to be the best encoding choice
   // here since it doesn't require an extra register.
   // 3) OR appears to be very slightly faster than ADD. (Though, the difference
   // is small enough it might just be measurement noise.)
-  // 4) For the moment, we are using top of stack.  This creates false sharing
-  // with actual stack access/call sequences, and it would be better to use a
-  // location within the redzone.  For the moment, this is still better than an
-  // mfence though.  TODO: Revise the offset used when we can assume a redzone.
+  // 4) When choosing offsets, there are several contributing factors:
+  //   a) If there's no redzone, we default to TOS.  (We could allocate a cache
+  //      line aligned stack object to improve this case.) 
+  //   b) To minimize our chances of introducing a false dependence, we prefer
+  //      to offset the stack usage from TOS slightly.  
+  //   c) To minimize concerns about cross thread stack usage - in particular,
+  //      the idiomatic MyThreadPool.run([&StackVars]() {...}) pattern which
+  //      captures state in the TOS frame and accesses it from many threads -
+  //      we want to use an offset such that the offset is in a distinct cache
+  //      line from the TOS frame.
   // 
   // For a general discussion of the tradeoffs and benchmark results, see:
   // https://shipilev.net/blog/2014/on-the-fence-with-dependencies/
 
+  auto &MF = DAG.getMachineFunction();
+  auto &TFL = *Subtarget.getFrameLowering();
+  const unsigned SPOffset = TFL.has128ByteRedZone(MF) ? -64 : 0;
+
   if (Subtarget.is64Bit()) {
     SDValue Zero = DAG.getTargetConstant(0, DL, MVT::i32);
     SDValue Ops[] = {
       DAG.getRegister(X86::RSP, MVT::i64),                  // Base
       DAG.getTargetConstant(1, DL, MVT::i8),                // Scale
       DAG.getRegister(0, MVT::i64),                         // Index
-      DAG.getTargetConstant(0, DL, MVT::i32),               // Disp
+      DAG.getTargetConstant(SPOffset, DL, MVT::i32),        // Disp
       DAG.getRegister(0, MVT::i16),                         // Segment.
       Zero,
       Chain};
     SDNode *Res = DAG.getMachineNode(X86::LOCK_OR32mi8, DL, MVT::i32,
                                      MVT::Other, Ops);
     return SDValue(Res, 1);
   }
 
   SDValue Zero = DAG.getTargetConstant(0, DL, MVT::i32);
   SDValue Ops[] = {
     DAG.getRegister(X86::ESP, MVT::i32),            // Base
     DAG.getTargetConstant(1, DL, MVT::i8),          // Scale
     DAG.getRegister(0, MVT::i32),                   // Index
-    DAG.getTargetConstant(0, DL, MVT::i32),         // Disp
+    DAG.getTargetConstant(SPOffset, DL, MVT::i32),  // Disp
     DAG.getRegister(0, MVT::i16),                   // Segment.
     Zero,
     Chain
   };
   SDNode *Res = DAG.getMachineNode(X86::LOCK_OR32mi8, DL, MVT::i32,
                                    MVT::Other, Ops);
   return SDValue(Res, 1);
 }

llvm/trunk/test/CodeGen/X86/atomic-idempotent.ll

 ; CHECK-NEXT:    ret{{[l|q]}}
   atomicrmw or i32* %p, i32 0 acq_rel
   ret void
 }
 
 define void @or32_nouse_seq_cst(i32* %p) {
 ; X64-LABEL: or32_nouse_seq_cst:
 ; X64:       # %bb.0:
-; X64-NEXT:    lock orl $0, (%rsp)
+; X64-NEXT:    lock orl $0, -{{[0-9]+}}(%rsp)
 ; X64-NEXT:    retq
 ;
 ; X86-LABEL: or32_nouse_seq_cst:
 ; X86:       # %bb.0:
 ; X86-NEXT:    lock orl $0, (%esp)
 ; X86-NEXT:    retl
   atomicrmw or i32* %p, i32 0 seq_cst
   ret void
 }
 
 ; TODO: The value isn't used on 32 bit, so the cmpxchg8b is unneeded
 define void @or64_nouse_seq_cst(i64* %p) {
 ; X64-LABEL: or64_nouse_seq_cst:
 ; X64:       # %bb.0:
-; X64-NEXT:    lock orl $0, (%rsp)
+; X64-NEXT:    lock orl $0, -{{[0-9]+}}(%rsp)
 ; X64-NEXT:    retq
 ;
 ; X86-LABEL: or64_nouse_seq_cst:
 ; X86:       # %bb.0:
 ; X86-NEXT:    pushl %ebx
 ; X86-NEXT:    .cfi_def_cfa_offset 8
 ; X86-NEXT:    pushl %esi
 ; X86-NEXT:    .cfi_def_cfa_offset 12
; X86-NEXT:    retl
   atomicrmw or i128* %p, i128 0 seq_cst
   ret void
 }
 
 
 define void @or16_nouse_seq_cst(i16* %p) {
 ; X64-LABEL: or16_nouse_seq_cst:
 ; X64:       # %bb.0:
-; X64-NEXT:    lock orl $0, (%rsp)
+; X64-NEXT:    lock orl $0, -{{[0-9]+}}(%rsp)
 ; X64-NEXT:    retq
 ;
 ; X86-LABEL: or16_nouse_seq_cst:
 ; X86:       # %bb.0:
 ; X86-NEXT:    lock orl $0, (%esp)
 ; X86-NEXT:    retl
   atomicrmw or i16* %p, i16 0 seq_cst
   ret void
 }
 
 define void @or8_nouse_seq_cst(i8* %p) {
 ; X64-LABEL: or8_nouse_seq_cst:
 ; X64:       # %bb.0:
-; X64-NEXT:    lock orl $0, (%rsp)
+; X64-NEXT:    lock orl $0, -{{[0-9]+}}(%rsp)
 ; X64-NEXT:    retq
 ;
 ; X86-LABEL: or8_nouse_seq_cst:
 ; X86:       # %bb.0:
 ; X86-NEXT:    lock orl $0, (%esp)
 ; X86-NEXT:    retl
   atomicrmw or i8* %p, i8 0 seq_cst
   ret void
 }

llvm/trunk/test/CodeGen/X86/speculative-load-hardening.ll

 ; Make sure we don't crash on idempotent atomic operations which have a
 ; hardcoded reference to RSP+offset.
 define void @idempotent_atomic(i32* %x) speculative_load_hardening {
 ; X64-LABEL: idempotent_atomic:
 ; X64:       # %bb.0:
 ; X64-NEXT:    movq %rsp, %rax
 ; X64-NEXT:    movq $-1, %rcx
 ; X64-NEXT:    sarq $63, %rax
-; X64-NEXT:    lock orl $0, (%rsp)
+; X64-NEXT:    lock orl $0, -64(%rsp)
 ; X64-NEXT:    shlq $47, %rax
 ; X64-NEXT:    orq %rax, %rsp
 ; X64-NEXT:    retq
 ;
 ; X64-LFENCE-LABEL: idempotent_atomic:
 ; X64-LFENCE:       # %bb.0:
-; X64-LFENCE-NEXT:    lock orl $0, (%rsp)
+; X64-LFENCE-NEXT:    lock orl $0, -64(%rsp)
 ; X64-LFENCE-NEXT:    retq
   %tmp = atomicrmw or i32* %x, i32 0 seq_cst
   ret void
 }