[AMDGPU] In SIMemoryLegalizer assume all atomic ops have memoperands

[foad]

Summary

Remove the conservative handling of maybeAtomic ops with no
memoperands. In practice this path is untested because all maybeAtomic
ops do have memoperands. Remove various unused default values for fields
in SIMemOpInfo.

If we add new ops without memoperands in future it's better to hit the
assertion in constructFromMIWithMMO early than to have them handled
conservatively (which is correct but slow) and only find out about the
performance problem later.

[arsenm]

This at least needs to be backed up with a verifier change.

Arguably this should be a general verifier property if we really can commit to machine passes never dropping MMOs

[t-tye]

My understanding was that it was not guaranteed that an MI instruction has a MMO. That passes could drop the MMO. Are you saying that is not the case? As Matt says, if that is the case the verifier needs to enforce that.

[foad]

In D115229#3177688, @arsenm wrote:

This at least needs to be backed up with a verifier change.

I'm not ready to assert that all mayLoad/mayStore MIs have an MMO. I'm only saying that this seems to be true:

• for MIs used by the AMDGPU backend,
• which have the AMDGPU-specific maybeAtomic flag set.

I don't really understand the point of the maybeAtomic flag, so I might work on removing that first and then revisit this patch.

Arguably this should be a general verifier property if we really can commit to machine passes never dropping MMOs

I wasn't aware that passes were allowed to drop MMOs. Do you know of any specific examples? It seems like a bit of an odd situation if they are allowed to, but it never actually happens in practice.

[arsenm]

In D115229#3178899, @foad wrote:

I wasn't aware that passes were allowed to drop MMOs. Do you know of any specific examples? It seems like a bit of an odd situation if they are allowed to, but it never actually happens in practice.

The verifier doesn't enforce an MMO requirement, so it's defacto. Plus hasUnorderedMemoryRef implies this is OK.

I think the concern would be if you did something like if convert 2 conditional loads into one load with a select on the pointer. You would have to common the memory operands somehow (which I guess would be dropping the IR reference and selecting the most conservative flags of the pair)

llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp

enum class SIAtomicAddrSpace {
   LLVM_MARK_AS_BITMASK_ENUM(/* LargestFlag = */ ALL)
 };
 
 class SIMemOpInfo final {
 private:
 
   friend class SIMemOpAccess;
 
-  AtomicOrdering Ordering = AtomicOrdering::NotAtomic;
-  AtomicOrdering FailureOrdering = AtomicOrdering::NotAtomic;
-  SIAtomicScope Scope = SIAtomicScope::SYSTEM;
-  SIAtomicAddrSpace OrderingAddrSpace = SIAtomicAddrSpace::NONE;
-  SIAtomicAddrSpace InstrAddrSpace = SIAtomicAddrSpace::NONE;
-  bool IsCrossAddressSpaceOrdering = false;
-  bool IsVolatile = false;
-  bool IsNonTemporal = false;
-
-  SIMemOpInfo(AtomicOrdering Ordering = AtomicOrdering::SequentiallyConsistent,
-              SIAtomicScope Scope = SIAtomicScope::SYSTEM,
-              SIAtomicAddrSpace OrderingAddrSpace = SIAtomicAddrSpace::ATOMIC,
-              SIAtomicAddrSpace InstrAddrSpace = SIAtomicAddrSpace::ALL,
-              bool IsCrossAddressSpaceOrdering = true,
-              AtomicOrdering FailureOrdering =
-                AtomicOrdering::SequentiallyConsistent,
-              bool IsVolatile = false,
-              bool IsNonTemporal = false)
-    : Ordering(Ordering), FailureOrdering(FailureOrdering),
-      Scope(Scope), OrderingAddrSpace(OrderingAddrSpace),
-      InstrAddrSpace(InstrAddrSpace),
+  AtomicOrdering Ordering;
+  AtomicOrdering FailureOrdering;
+  SIAtomicScope Scope;
+  SIAtomicAddrSpace OrderingAddrSpace;
+  SIAtomicAddrSpace InstrAddrSpace;
+  bool IsCrossAddressSpaceOrdering;
+  bool IsVolatile;
+  bool IsNonTemporal;
+
+  SIMemOpInfo(AtomicOrdering Ordering, SIAtomicScope Scope,
+              SIAtomicAddrSpace OrderingAddrSpace,
+              SIAtomicAddrSpace InstrAddrSpace,
+              bool IsCrossAddressSpaceOrdering, AtomicOrdering FailureOrdering,
+              bool IsVolatile = false, bool IsNonTemporal = false)
+      : Ordering(Ordering), FailureOrdering(FailureOrdering), Scope(Scope),
+        OrderingAddrSpace(OrderingAddrSpace), InstrAddrSpace(InstrAddrSpace),
         IsCrossAddressSpaceOrdering(IsCrossAddressSpaceOrdering),
-      IsVolatile(IsVolatile),
-      IsNonTemporal(IsNonTemporal) {
+        IsVolatile(IsVolatile), IsNonTemporal(IsNonTemporal) {
 
     if (Ordering == AtomicOrdering::NotAtomic) {
       assert(Scope == SIAtomicScope::NONE &&
              OrderingAddrSpace == SIAtomicAddrSpace::NONE &&
              !IsCrossAddressSpaceOrdering &&
              FailureOrdering == AtomicOrdering::NotAtomic);
       return;
     }

 
 Optional<SIMemOpInfo> SIMemOpAccess::getLoadInfo(
     const MachineBasicBlock::iterator &MI) const {
   assert(MI->getDesc().TSFlags & SIInstrFlags::maybeAtomic);
 
   if (!(MI->mayLoad() && !MI->mayStore()))
     return None;
 
-  // Be conservative if there are no memory operands.
-  if (MI->getNumMemOperands() == 0)
-    return SIMemOpInfo();
-
   return constructFromMIWithMMO(MI);
 }
 
 Optional<SIMemOpInfo> SIMemOpAccess::getStoreInfo(
     const MachineBasicBlock::iterator &MI) const {
   assert(MI->getDesc().TSFlags & SIInstrFlags::maybeAtomic);
 
   if (!(!MI->mayLoad() && MI->mayStore()))
     return None;
 
-  // Be conservative if there are no memory operands.
-  if (MI->getNumMemOperands() == 0)
-    return SIMemOpInfo();
-
   return constructFromMIWithMMO(MI);
 }
 
 Optional<SIMemOpInfo> SIMemOpAccess::getAtomicFenceInfo(
     const MachineBasicBlock::iterator &MI) const {
   assert(MI->getDesc().TSFlags & SIInstrFlags::maybeAtomic);
 
   if (MI->getOpcode() != AMDGPU::ATOMIC_FENCE)

 
 Optional<SIMemOpInfo> SIMemOpAccess::getAtomicCmpxchgOrRmwInfo(
     const MachineBasicBlock::iterator &MI) const {
   assert(MI->getDesc().TSFlags & SIInstrFlags::maybeAtomic);
 
   if (!(MI->mayLoad() && MI->mayStore()))
     return None;
 
-  // Be conservative if there are no memory operands.
-  if (MI->getNumMemOperands() == 0)
-    return SIMemOpInfo();
-
   return constructFromMIWithMMO(MI);
 }
 
 SICacheControl::SICacheControl(const GCNSubtarget &ST) : ST(ST) {
   TII = ST.getInstrInfo();
   IV = getIsaVersion(ST.getCPU());
   InsertCacheInv = !AmdgcnSkipCacheInvalidations;
 }

llvm/test/CodeGen/AMDGPU/expand-si-indirect.mir

bb.0.entry:
     %23:vreg_512 = V_INDIRECT_REG_WRITE_GPR_IDX_B32_V16 killed %21, %22, killed %4, 3, implicit-def $m0, implicit $m0, implicit $exec
     %24:sreg_32 = S_ADD_I32 killed %3, 2, implicit-def dead $scc
     %25:vreg_512 = V_INDIRECT_REG_WRITE_GPR_IDX_B32_V16 %23, killed %22, killed %24, 3, implicit-def $m0, implicit $m0, implicit $exec
     %26:vgpr_32 = COPY %23.sub15
     %27:vgpr_32 = COPY %23.sub14
     %28:vgpr_32 = COPY %23.sub13
     %29:vgpr_32 = COPY %23.sub12
     %30:vreg_128 = REG_SEQUENCE killed %29, %subreg.sub0, killed %28, %subreg.sub1, killed %27, %subreg.sub2, killed %26, %subreg.sub3
-    GLOBAL_STORE_DWORDX4_SADDR %1, killed %30, %2, 48, 0, implicit $exec
+    GLOBAL_STORE_DWORDX4_SADDR %1, killed %30, %2, 48, 0, implicit $exec :: (store 16)
     %31:vgpr_32 = COPY %23.sub11
     %32:vgpr_32 = COPY %23.sub10
     %33:vgpr_32 = COPY %23.sub9
     %34:vgpr_32 = COPY %23.sub8
     %35:vreg_128 = REG_SEQUENCE killed %34, %subreg.sub0, killed %33, %subreg.sub1, killed %32, %subreg.sub2, killed %31, %subreg.sub3
-    GLOBAL_STORE_DWORDX4_SADDR %1, killed %35, %2, 32, 0, implicit $exec
+    GLOBAL_STORE_DWORDX4_SADDR %1, killed %35, %2, 32, 0, implicit $exec :: (store 16)
     %36:vgpr_32 = COPY %23.sub7
     %37:vgpr_32 = COPY %23.sub6
     %38:vgpr_32 = COPY %23.sub5
     %39:vgpr_32 = COPY %23.sub4
     %40:vreg_128 = REG_SEQUENCE killed %39, %subreg.sub0, killed %38, %subreg.sub1, killed %37, %subreg.sub2, killed %36, %subreg.sub3
-    GLOBAL_STORE_DWORDX4_SADDR %1, killed %40, %2, 16, 0, implicit $exec
+    GLOBAL_STORE_DWORDX4_SADDR %1, killed %40, %2, 16, 0, implicit $exec :: (store 16)
     %41:vgpr_32 = COPY %23.sub3
     %42:vgpr_32 = COPY %23.sub2
     %43:vgpr_32 = COPY %23.sub1
     %44:vgpr_32 = COPY killed %23.sub0
     %45:vreg_128 = REG_SEQUENCE killed %44, %subreg.sub0, killed %43, %subreg.sub1, killed %42, %subreg.sub2, killed %41, %subreg.sub3
-    GLOBAL_STORE_DWORDX4_SADDR %1, killed %45, %2, 0, 0, implicit $exec
+    GLOBAL_STORE_DWORDX4_SADDR %1, killed %45, %2, 0, 0, implicit $exec :: (store 16)
     %46:vgpr_32 = COPY %25.sub15
     %47:vgpr_32 = COPY %25.sub14
     %48:vgpr_32 = COPY %25.sub13
     %49:vgpr_32 = COPY %25.sub12
     %50:vreg_128 = REG_SEQUENCE killed %49, %subreg.sub0, killed %48, %subreg.sub1, killed %47, %subreg.sub2, killed %46, %subreg.sub3
-    GLOBAL_STORE_DWORDX4_SADDR %1, killed %50, %2, 112, 0, implicit $exec
+    GLOBAL_STORE_DWORDX4_SADDR %1, killed %50, %2, 112, 0, implicit $exec :: (store 16)
     %51:vgpr_32 = COPY %25.sub11
     %52:vgpr_32 = COPY %25.sub10
     %53:vgpr_32 = COPY %25.sub9
     %54:vgpr_32 = COPY %25.sub8
     %55:vreg_128 = REG_SEQUENCE killed %54, %subreg.sub0, killed %53, %subreg.sub1, killed %52, %subreg.sub2, killed %51, %subreg.sub3
-    GLOBAL_STORE_DWORDX4_SADDR %1, killed %55, %2, 96, 0, implicit $exec
+    GLOBAL_STORE_DWORDX4_SADDR %1, killed %55, %2, 96, 0, implicit $exec :: (store 16)
     %56:vgpr_32 = COPY %25.sub7
     %57:vgpr_32 = COPY %25.sub6
     %58:vgpr_32 = COPY %25.sub5
     %59:vgpr_32 = COPY %25.sub4
     %60:vreg_128 = REG_SEQUENCE killed %59, %subreg.sub0, killed %58, %subreg.sub1, killed %57, %subreg.sub2, killed %56, %subreg.sub3
-    GLOBAL_STORE_DWORDX4_SADDR %1, killed %60, %2, 80, 0, implicit $exec
+    GLOBAL_STORE_DWORDX4_SADDR %1, killed %60, %2, 80, 0, implicit $exec :: (store 16)
     %61:vgpr_32 = COPY %25.sub3
     %62:vgpr_32 = COPY %25.sub2
     %63:vgpr_32 = COPY %25.sub1
     %64:vgpr_32 = COPY killed %25.sub0
     %65:vreg_128 = REG_SEQUENCE killed %64, %subreg.sub0, killed %63, %subreg.sub1, killed %62, %subreg.sub2, killed %61, %subreg.sub3
-    GLOBAL_STORE_DWORDX4_SADDR killed %1, killed %65, killed %2, 64, 0, implicit $exec
+    GLOBAL_STORE_DWORDX4_SADDR killed %1, killed %65, killed %2, 64, 0, implicit $exec :: (store 16)
     S_ENDPGM 0
 
 ...
 

llvm/test/CodeGen/AMDGPU/shrink-carry.mir

 body:             |
   bb.0:
 
     %0 = IMPLICIT_DEF
     %1 = IMPLICIT_DEF
     %2 = IMPLICIT_DEF
     %3 = V_CMP_GT_U32_e64 %0, %1, implicit $exec
     %4, %5 = V_SUBBREV_U32_e64 0, %0, %3, 0, implicit $exec
-    GLOBAL_STORE_DWORD undef $vgpr0_vgpr1, %4, 0, 0, implicit $exec
+    GLOBAL_STORE_DWORD undef $vgpr0_vgpr1, %4, 0, 0, implicit $exec :: (store 4)
 
 ...
 
 # GCN-LABEL: name: subb{{$}}
 # GCN:       V_SUBBREV_U32_e32 0, undef $vgpr0, implicit-def dead $vcc, implicit killed $vcc, implicit $exec
 
 ---
 name:            subb
 tracksRegLiveness: true
 registers:
   - { id: 0, class: vgpr_32 }
   - { id: 1, class: vgpr_32 }
   - { id: 2, class: vgpr_32 }
   - { id: 3, class: sreg_64_xexec }
   - { id: 4, class: vgpr_32 }
   - { id: 5, class: sreg_64_xexec }
 body:             |
   bb.0:
 
     %0 = IMPLICIT_DEF
     %1 = IMPLICIT_DEF
     %2 = IMPLICIT_DEF
     %3 = V_CMP_GT_U32_e64 %0, %1, implicit $exec
     %4, %5 = V_SUBB_U32_e64 %0, 0, %3, 0, implicit $exec
-    GLOBAL_STORE_DWORD undef $vgpr0_vgpr1, %4, 0, 0, implicit $exec
+    GLOBAL_STORE_DWORD undef $vgpr0_vgpr1, %4, 0, 0, implicit $exec :: (store 4)
 
 ...
 
 # GCN-LABEL: name: addc{{$}}
 # GCN:       V_ADDC_U32_e32 0, undef $vgpr0, implicit-def dead $vcc, implicit killed $vcc, implicit $exec
 
 ---
 name:            addc
 tracksRegLiveness: true
 registers:
   - { id: 0, class: vgpr_32 }
   - { id: 1, class: vgpr_32 }
   - { id: 2, class: vgpr_32 }
   - { id: 3, class: sreg_64_xexec }
   - { id: 4, class: vgpr_32 }
   - { id: 5, class: sreg_64_xexec }
 body:             |
   bb.0:
 
     %0 = IMPLICIT_DEF
     %1 = IMPLICIT_DEF
     %2 = IMPLICIT_DEF
     %3 = V_CMP_GT_U32_e64 %0, %1, implicit $exec
     %4, %5 = V_ADDC_U32_e64 0, %0, %3, 0, implicit $exec
-    GLOBAL_STORE_DWORD undef $vgpr0_vgpr1, %4, 0, 0, implicit $exec
+    GLOBAL_STORE_DWORD undef $vgpr0_vgpr1, %4, 0, 0, implicit $exec :: (store 4)
 
 ...
 
 # GCN-LABEL: name: addc2{{$}}
 # GCN:       V_ADDC_U32_e32 0, undef $vgpr0, implicit-def dead $vcc, implicit killed $vcc, implicit $exec
 
 ---
 name:            addc2
 tracksRegLiveness: true
 registers:
   - { id: 0, class: vgpr_32 }
   - { id: 1, class: vgpr_32 }
   - { id: 2, class: vgpr_32 }
   - { id: 3, class: sreg_64_xexec }
   - { id: 4, class: vgpr_32 }
   - { id: 5, class: sreg_64_xexec }
 body:             |
   bb.0:
 
     %0 = IMPLICIT_DEF
     %1 = IMPLICIT_DEF
     %2 = IMPLICIT_DEF
     %3 = V_CMP_GT_U32_e64 %0, %1, implicit $exec
     %4, %5 = V_ADDC_U32_e64 %0, 0, %3, 0, implicit $exec
-    GLOBAL_STORE_DWORD undef $vgpr0_vgpr1, %4, 0, 0, implicit $exec
+    GLOBAL_STORE_DWORD undef $vgpr0_vgpr1, %4, 0, 0, implicit $exec :: (store 4)
 
 ...

llvm/test/CodeGen/AMDGPU/subvector-test.mir

bb.1:
     successors: %bb.1, %bb.2
 
     %14:sreg_32_xm0 = S_ADD_I32 %11.sub0, %11.sub1, implicit-def dead $scc
     %16:vgpr_32 = COPY %14
     S_SUBVECTOR_LOOP_END %bb.1, %19:sreg_32, implicit-def $exec, implicit $exec, implicit-def %19
 
   bb.2:
 
-    GLOBAL_STORE_DWORD %15, %16, 0, 0, implicit $exec
+    GLOBAL_STORE_DWORD %15, %16, 0, 0, implicit $exec :: (store 4)
     S_ENDPGM 0
 ...