[X86][GlobalISEL] Support lowering aligned unordered atomics

[reames]

Summary

This is a companion to D57601, but will be landed afterwards. I don't want to have to worry about two sources of atomic MMOs, some volatile, some not.

The existing lowering code is accidentally correct for unordered atomics as far as I can tell. An unordered atomic has no memory ordering, and simply requires the actual load or store to be done as a single well aligned instruction. As such, relax the restriction while adding tests to ensure the lowering remains correct in the future.

[arsenm]

test/CodeGen/X86/atomic-unordered.ll

2–6

I don't know about mixing GlobalISel and SelectionDAG tests.

I would also expect this to be a MIR pass which just runs the selector, but it looks like x86 is using a number of end-to-end tests for this already

[jlebar]

This seems reasonable to me but I'm not familiar enough with the x86 backend to feel comfortable signing off on it.

[igorb]

The change looks ok.
Could you please add mir test for InstructionSelector pass, similar to select-memop-scalar.mir.
Thanks

[reames]

In D57803#1388399, @igorb wrote:

The change looks ok.
Could you please add mir test for InstructionSelector pass, similar to select-memop-scalar.mir.
Thanks

Went ahead and landed this with the mir test as requested, but honestly, I think the mir test is much less useful and readable than a plain IR test. The sheer amount of *duplication* in the MIR file was distressing.

What I'd really have preferred - for global ISEL specific tests - was a version which started with IR, but used -stop-after=instruction-selector. I couldn't find a way to do that w/an auto updated test though.

lib/Target/X86/X86InstructionSelector.cpp

bool X86InstructionSelector::selectLoadStoreOp(MachineInstr &I,
 
   assert((Opc == TargetOpcode::G_STORE || Opc == TargetOpcode::G_LOAD) &&
          "unexpected instruction");
 
   const unsigned DefReg = I.getOperand(0).getReg();
   LLT Ty = MRI.getType(DefReg);
   const RegisterBank &RB = *RBI.getRegBank(DefReg, MRI, TRI);
 
+  assert(I.hasOneMemOperand());
   auto &MemOp = **I.memoperands_begin();
-  if (MemOp.getOrdering() != AtomicOrdering::NotAtomic) {
-    LLVM_DEBUG(dbgs() << "Atomic load/store not supported yet\n");
+  if (MemOp.isAtomic()) {
+    // Note: for unordered operations, we rely on the fact the appropriate MMO
+    // is already on the instruction we're mutating, and thus we don't need to
+    // make any changes.  So long as we select an opcode which is capable of
+    // loading or storing the appropriate size atomically, the rest of the
+    // backend is required to respect the MMO state. 
+    if (!MemOp.isUnordered()) {
+      LLVM_DEBUG(dbgs() << "Atomic ordering not supported yet\n");
       return false;
     }
+    if (MemOp.getAlignment() < Ty.getSizeInBits()/8) {
+      LLVM_DEBUG(dbgs() << "Unaligned atomics not supported yet\n");
+      return false;
+    }
+  }
 
   unsigned NewOpc = getLoadStoreOp(Ty, RB, Opc, MemOp.getAlignment());
   if (NewOpc == Opc)
     return false;
 
   X86AddressMode AM;
   X86SelectAddress(*MRI.getVRegDef(I.getOperand(1).getReg()), MRI, AM);
 

test/CodeGen/X86/atomic-unordered.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -O0 < %s -mtriple=x86_64-linux-generic -verify-machineinstrs -mattr=sse2 | FileCheck --check-prefix=CHECK-O0 %s
+; RUN: llc -O0 -global-isel < %s -mtriple=x86_64-linux-generic -verify-machineinstrs -mattr=sse2 | FileCheck --check-prefix=CHECK-O0-GISEL %s
 ; RUN: llc -O3 < %s -mtriple=x86_64-linux-generic -verify-machineinstrs -mattr=sse2 | FileCheck --check-prefix=CHECK-O3 %s
+; RUN: llc -O3 -global-isel < %s -mtriple=x86_64-linux-generic -verify-machineinstrs -mattr=sse2 | FileCheck --check-prefix=CHECK-O3-GISEL %s
 
 define i8 @load_i8(i8* %ptr) {
 ; CHECK-O0-LABEL: load_i8:
 ; CHECK-O0:       # %bb.0:
 ; CHECK-O0-NEXT:    movb (%rdi), %al
 ; CHECK-O0-NEXT:    retq
 ;
+; CHECK-O0-GISEL-LABEL: load_i8:
+; CHECK-O0-GISEL:       # %bb.1:
+; CHECK-O0-GISEL-NEXT:    movb (%rdi), %al
+; CHECK-O0-GISEL-NEXT:    retq
+;
 ; CHECK-O3-LABEL: load_i8:
 ; CHECK-O3:       # %bb.0:
 ; CHECK-O3-NEXT:    movb (%rdi), %al
 ; CHECK-O3-NEXT:    retq
+;
+; CHECK-O3-GISEL-LABEL: load_i8:
+; CHECK-O3-GISEL:       # %bb.0:
+; CHECK-O3-GISEL-NEXT:    movb (%rdi), %al
+; CHECK-O3-GISEL-NEXT:    retq
   %v = load atomic i8, i8* %ptr unordered, align 1
   ret i8 %v
 }
 
 define void @store_i8(i8* %ptr, i8 %v) {
 ; CHECK-O0-LABEL: store_i8:
 ; CHECK-O0:       # %bb.0:
 ; CHECK-O0-NEXT:    movb %sil, %al
 ; CHECK-O0-NEXT:    movb %al, (%rdi)
 ; CHECK-O0-NEXT:    retq
 ;
+; CHECK-O0-GISEL-LABEL: store_i8:
+; CHECK-O0-GISEL:       # %bb.1:
+; CHECK-O0-GISEL-NEXT:    movb %sil, %al
+; CHECK-O0-GISEL-NEXT:    movb %al, (%rdi)
+; CHECK-O0-GISEL-NEXT:    retq
+;
 ; CHECK-O3-LABEL: store_i8:
 ; CHECK-O3:       # %bb.0:
 ; CHECK-O3-NEXT:    movb %sil, (%rdi)
 ; CHECK-O3-NEXT:    retq
+;
+; CHECK-O3-GISEL-LABEL: store_i8:
+; CHECK-O3-GISEL:       # %bb.0:
+; CHECK-O3-GISEL-NEXT:    movb %sil, (%rdi)
+; CHECK-O3-GISEL-NEXT:    retq
   store atomic i8 %v, i8* %ptr unordered, align 1
   ret void
 }
 
 define i16 @load_i16(i16* %ptr) {
 ; CHECK-O0-LABEL: load_i16:
 ; CHECK-O0:       # %bb.0:
 ; CHECK-O0-NEXT:    movw (%rdi), %ax
 ; CHECK-O0-NEXT:    retq
 ;
+; CHECK-O0-GISEL-LABEL: load_i16:
+; CHECK-O0-GISEL:       # %bb.1:
+; CHECK-O0-GISEL-NEXT:    movw (%rdi), %ax
+; CHECK-O0-GISEL-NEXT:    retq
+;
 ; CHECK-O3-LABEL: load_i16:
 ; CHECK-O3:       # %bb.0:
 ; CHECK-O3-NEXT:    movzwl (%rdi), %eax
 ; CHECK-O3-NEXT:    retq
+;
+; CHECK-O3-GISEL-LABEL: load_i16:
+; CHECK-O3-GISEL:       # %bb.0:
+; CHECK-O3-GISEL-NEXT:    movzwl (%rdi), %eax
+; CHECK-O3-GISEL-NEXT:    retq
   %v = load atomic i16, i16* %ptr unordered, align 2
   ret i16 %v
 }
 
 
 define void @store_i16(i16* %ptr, i16 %v) {
 ; CHECK-O0-LABEL: store_i16:
 ; CHECK-O0:       # %bb.0:
 ; CHECK-O0-NEXT:    movw %si, %ax
 ; CHECK-O0-NEXT:    movw %ax, (%rdi)
 ; CHECK-O0-NEXT:    retq
 ;
+; CHECK-O0-GISEL-LABEL: store_i16:
+; CHECK-O0-GISEL:       # %bb.1:
+; CHECK-O0-GISEL-NEXT:    movw %si, %ax
+; CHECK-O0-GISEL-NEXT:    movw %ax, (%rdi)
+; CHECK-O0-GISEL-NEXT:    retq
+;
 ; CHECK-O3-LABEL: store_i16:
 ; CHECK-O3:       # %bb.0:
 ; CHECK-O3-NEXT:    movw %si, (%rdi)
 ; CHECK-O3-NEXT:    retq
+;
+; CHECK-O3-GISEL-LABEL: store_i16:
+; CHECK-O3-GISEL:       # %bb.0:
+; CHECK-O3-GISEL-NEXT:    movw %si, (%rdi)
+; CHECK-O3-GISEL-NEXT:    retq
   store atomic i16 %v, i16* %ptr unordered, align 2
   ret void
 }
 
 define i32 @load_i32(i32* %ptr) {
 ; CHECK-O0-LABEL: load_i32:
 ; CHECK-O0:       # %bb.0:
 ; CHECK-O0-NEXT:    movl (%rdi), %eax
 ; CHECK-O0-NEXT:    retq
 ;
+; CHECK-O0-GISEL-LABEL: load_i32:
+; CHECK-O0-GISEL:       # %bb.1:
+; CHECK-O0-GISEL-NEXT:    movl (%rdi), %eax
+; CHECK-O0-GISEL-NEXT:    retq
+;
 ; CHECK-O3-LABEL: load_i32:
 ; CHECK-O3:       # %bb.0:
 ; CHECK-O3-NEXT:    movl (%rdi), %eax
 ; CHECK-O3-NEXT:    retq
+;
+; CHECK-O3-GISEL-LABEL: load_i32:
+; CHECK-O3-GISEL:       # %bb.0:
+; CHECK-O3-GISEL-NEXT:    movl (%rdi), %eax
+; CHECK-O3-GISEL-NEXT:    retq
   %v = load atomic i32, i32* %ptr unordered, align 4
   ret i32 %v
 }
 
 define void @store_i32(i32* %ptr, i32 %v) {
 ; CHECK-O0-LABEL: store_i32:
 ; CHECK-O0:       # %bb.0:
 ; CHECK-O0-NEXT:    movl %esi, (%rdi)
 ; CHECK-O0-NEXT:    retq
 ;
+; CHECK-O0-GISEL-LABEL: store_i32:
+; CHECK-O0-GISEL:       # %bb.1:
+; CHECK-O0-GISEL-NEXT:    movl %esi, (%rdi)
+; CHECK-O0-GISEL-NEXT:    retq
+;
 ; CHECK-O3-LABEL: store_i32:
 ; CHECK-O3:       # %bb.0:
 ; CHECK-O3-NEXT:    movl %esi, (%rdi)
 ; CHECK-O3-NEXT:    retq
+;
+; CHECK-O3-GISEL-LABEL: store_i32:
+; CHECK-O3-GISEL:       # %bb.0:
+; CHECK-O3-GISEL-NEXT:    movl %esi, (%rdi)
+; CHECK-O3-GISEL-NEXT:    retq
   store atomic i32 %v, i32* %ptr unordered, align 4
   ret void
 }
 
 define i64 @load_i64(i64* %ptr) {
 ; CHECK-O0-LABEL: load_i64:
 ; CHECK-O0:       # %bb.0:
 ; CHECK-O0-NEXT:    movq (%rdi), %rax
 ; CHECK-O0-NEXT:    retq
 ;
+; CHECK-O0-GISEL-LABEL: load_i64:
+; CHECK-O0-GISEL:       # %bb.1:
+; CHECK-O0-GISEL-NEXT:    movq (%rdi), %rax
+; CHECK-O0-GISEL-NEXT:    retq
+;
 ; CHECK-O3-LABEL: load_i64:
 ; CHECK-O3:       # %bb.0:
 ; CHECK-O3-NEXT:    movq (%rdi), %rax
 ; CHECK-O3-NEXT:    retq
+;
+; CHECK-O3-GISEL-LABEL: load_i64:
+; CHECK-O3-GISEL:       # %bb.0:
+; CHECK-O3-GISEL-NEXT:    movq (%rdi), %rax
+; CHECK-O3-GISEL-NEXT:    retq
   %v = load atomic i64, i64* %ptr unordered, align 8
   ret i64 %v
 }
 
 define void @store_i64(i64* %ptr, i64 %v) {
 ; CHECK-O0-LABEL: store_i64:
 ; CHECK-O0:       # %bb.0:
 ; CHECK-O0-NEXT:    movq %rsi, (%rdi)
 ; CHECK-O0-NEXT:    retq
 ;
+; CHECK-O0-GISEL-LABEL: store_i64:
+; CHECK-O0-GISEL:       # %bb.1:
+; CHECK-O0-GISEL-NEXT:    movq %rsi, (%rdi)
+; CHECK-O0-GISEL-NEXT:    retq
+;
 ; CHECK-O3-LABEL: store_i64:
 ; CHECK-O3:       # %bb.0:
 ; CHECK-O3-NEXT:    movq %rsi, (%rdi)
 ; CHECK-O3-NEXT:    retq
+;
+; CHECK-O3-GISEL-LABEL: store_i64:
+; CHECK-O3-GISEL:       # %bb.0:
+; CHECK-O3-GISEL-NEXT:    movq %rsi, (%rdi)
+; CHECK-O3-GISEL-NEXT:    retq
   store atomic i64 %v, i64* %ptr unordered, align 8
   ret void
 }