[AMDGPU] Reorder SIMemoryLegalizer functions to be consistent

[t-tye]

Summary

• Make the SIMemoryLegalizer insertAcquire function be in the same order for each target to be consistent.

[scott.linder]

One nit, could you add "[NFC]" to the commit message before committing?

[kzhuravl]

LGTM

[nhaehnle]

If we're nitpicking over such details, I would point out that it would be better to structure the whole thing differently, with a single legalizer class in which the insertAcquire etc. methods have different paths based on the hardware generation.

The reasoning is that code of the form "gfx10 behaves like gfx7 except ABC, gfx7 behaves like gfx6 except DEF" becomes increasingly difficult to reason about or refactor the more hardware generations we add.

llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp

virtual bool enableLoadCacheBypass(const MachineBasicBlock::iterator &MI,
                                      SIAtomicScope Scope,
                                      SIAtomicAddrSpace AddrSpace) const = 0;
 
   /// Update \p MI memory instruction to indicate it is
   /// nontemporal. Return true iff the instruction was modified.
   virtual bool enableNonTemporal(const MachineBasicBlock::iterator &MI)
     const = 0;
 
-  /// Inserts any necessary instructions at position \p Pos relative to
-  /// instruction \p MI to ensure any subsequent memory instructions of this
-  /// thread with address spaces \p AddrSpace will observe the previous memory
-  /// operations by any thread for memory scopes up to memory scope \p Scope .
-  /// Returns true iff any instructions inserted.
-  virtual bool insertAcquire(MachineBasicBlock::iterator &MI,
-                             SIAtomicScope Scope,
-                             SIAtomicAddrSpace AddrSpace,
-                             Position Pos) const = 0;
-
   /// Inserts any necessary instructions at position \p Pos relative
   /// to instruction \p MI to ensure memory instructions before \p Pos of kind
   /// \p Op associated with address spaces \p AddrSpace have completed. Used
   /// between memory instructions to enforce the order they become visible as
   /// observed by other memory instructions executing in memory scope \p Scope.
   /// \p IsCrossAddrSpaceOrdering indicates if the memory ordering is between
   /// address spaces. Returns true iff any instructions inserted.
   virtual bool insertWait(MachineBasicBlock::iterator &MI,
                           SIAtomicScope Scope,
                           SIAtomicAddrSpace AddrSpace,
                           SIMemOp Op,
                           bool IsCrossAddrSpaceOrdering,
                           Position Pos) const = 0;
 
   /// Inserts any necessary instructions at position \p Pos relative to
+  /// instruction \p MI to ensure any subsequent memory instructions of this
+  /// thread with address spaces \p AddrSpace will observe the previous memory
+  /// operations by any thread for memory scopes up to memory scope \p Scope .
+  /// Returns true iff any instructions inserted.
+  virtual bool insertAcquire(MachineBasicBlock::iterator &MI,
+                             SIAtomicScope Scope,
+                             SIAtomicAddrSpace AddrSpace,
+                             Position Pos) const = 0;
+
+  /// Inserts any necessary instructions at position \p Pos relative to
   /// instruction \p MI to ensure previous memory instructions by this thread
   /// with address spaces \p AddrSpace have completed and can be observed by
   /// subsequent memory instructions by any thread executing in memory scope \p
   /// Scope. \p IsCrossAddrSpaceOrdering indicates if the memory ordering is
   /// between address spaces. Returns true iff any instructions inserted.
   virtual bool insertRelease(MachineBasicBlock::iterator &MI,
                              SIAtomicScope Scope,
                              SIAtomicAddrSpace AddrSpace,
public:
   SIGfx6CacheControl(const GCNSubtarget &ST) : SICacheControl(ST) {};
 
   bool enableLoadCacheBypass(const MachineBasicBlock::iterator &MI,
                              SIAtomicScope Scope,
                              SIAtomicAddrSpace AddrSpace) const override;
 
   bool enableNonTemporal(const MachineBasicBlock::iterator &MI) const override;
 
-  bool insertAcquire(MachineBasicBlock::iterator &MI,
-                     SIAtomicScope Scope,
-                     SIAtomicAddrSpace AddrSpace,
-                     Position Pos) const override;
+  bool insertWait(MachineBasicBlock::iterator &MI,
+                  SIMemOp Op,
+                  bool IsCrossAddrSpaceOrdering,
 
-  bool insertRelease(MachineBasicBlock::iterator &MI,
+  bool insertAcquire(MachineBasicBlock::iterator &MI,
                      SIAtomicScope Scope,
                      SIAtomicAddrSpace AddrSpace,
-                     bool IsCrossAddrSpaceOrdering,
                      Position Pos) const override;
 
-  bool insertWait(MachineBasicBlock::iterator &MI,
+  bool insertRelease(MachineBasicBlock::iterator &MI,
                      SIAtomicScope Scope,
                      SIAtomicAddrSpace AddrSpace,
-                  SIMemOp Op,
                      bool IsCrossAddrSpaceOrdering,
                      Position Pos) const override;
 };
 
 class SIGfx7CacheControl : public SIGfx6CacheControl {
 public:
 
   SIGfx7CacheControl(const GCNSubtarget &ST) : SIGfx6CacheControl(ST) {};
 
   bool insertAcquire(MachineBasicBlock::iterator &MI,
public:
   SIGfx10CacheControl(const GCNSubtarget &ST) : SIGfx7CacheControl(ST) {};
 
   bool enableLoadCacheBypass(const MachineBasicBlock::iterator &MI,
                              SIAtomicScope Scope,
                              SIAtomicAddrSpace AddrSpace) const override;
 
   bool enableNonTemporal(const MachineBasicBlock::iterator &MI) const override;
 
-  bool insertAcquire(MachineBasicBlock::iterator &MI,
-                     SIAtomicScope Scope,
-                     SIAtomicAddrSpace AddrSpace,
-                     Position Pos) const override;
+  bool insertWait(MachineBasicBlock::iterator &MI,
+                  SIMemOp Op,
+                  bool IsCrossAddrSpaceOrdering,
 
-  bool insertWait(MachineBasicBlock::iterator &MI,
+  bool insertAcquire(MachineBasicBlock::iterator &MI,
                      SIAtomicScope Scope,
                      SIAtomicAddrSpace AddrSpace,
-                  SIMemOp Op,
-                  bool IsCrossAddrSpaceOrdering,
                      Position Pos) const override;
 };
 
 class SIMemoryLegalizer final : public MachineFunctionPass {
 private:
 
   /// Cache Control.
   std::unique_ptr<SICacheControl> CC = nullptr;
 
bool SIGfx6CacheControl::enableNonTemporal(
 
   /// TODO: Do not enableGLCBit if rmw atomic.
   Changed |= enableGLCBit(MI);
   Changed |= enableSLCBit(MI);
 
   return Changed;
 }
 
-bool SIGfx6CacheControl::insertAcquire(MachineBasicBlock::iterator &MI,
-                                       SIAtomicScope Scope,
-                                       SIAtomicAddrSpace AddrSpace,
-                                       Position Pos) const {
-  if (!InsertCacheInv)
-    return false;
-
-  bool Changed = false;
-
-  MachineBasicBlock &MBB = *MI->getParent();
-  DebugLoc DL = MI->getDebugLoc();
-
-  if (Pos == Position::AFTER)
-    ++MI;
-
-  if ((AddrSpace & SIAtomicAddrSpace::GLOBAL) != SIAtomicAddrSpace::NONE) {
-    switch (Scope) {
-    case SIAtomicScope::SYSTEM:
-    case SIAtomicScope::AGENT:
-      BuildMI(MBB, MI, DL, TII->get(AMDGPU::BUFFER_WBINVL1));
-      Changed = true;
-      break;
-    case SIAtomicScope::WORKGROUP:
-    case SIAtomicScope::WAVEFRONT:
-    case SIAtomicScope::SINGLETHREAD:
-      // No cache to invalidate.
-      break;
-    default:
-      llvm_unreachable("Unsupported synchronization scope");
-    }
-  }
-
-  /// The scratch address space does not need the global memory cache
-  /// to be flushed as all memory operations by the same thread are
-  /// sequentially consistent, and no other thread can access scratch
-  /// memory.
-
-  /// Other address spaces do not have a cache.
-
-  if (Pos == Position::AFTER)
-    --MI;
-
-  return Changed;
-}
-
 bool SIGfx6CacheControl::insertWait(MachineBasicBlock::iterator &MI,
                                     SIAtomicScope Scope,
                                     SIAtomicAddrSpace AddrSpace,
                                     SIMemOp Op,
                                     bool IsCrossAddrSpaceOrdering,
                                     Position Pos) const {
   bool Changed = false;
 
bool SIGfx6CacheControl::insertWait(MachineBasicBlock::iterator &MI,
   }
 
   if (Pos == Position::AFTER)
     --MI;
 
   return Changed;
 }
 
+bool SIGfx6CacheControl::insertAcquire(MachineBasicBlock::iterator &MI,
+                                       SIAtomicScope Scope,
+                                       SIAtomicAddrSpace AddrSpace,
+                                       Position Pos) const {
+  if (!InsertCacheInv)
+    return false;
+
+  bool Changed = false;
+
+  MachineBasicBlock &MBB = *MI->getParent();
+  DebugLoc DL = MI->getDebugLoc();
+
+  if (Pos == Position::AFTER)
+    ++MI;
+
+  if ((AddrSpace & SIAtomicAddrSpace::GLOBAL) != SIAtomicAddrSpace::NONE) {
+    switch (Scope) {
+    case SIAtomicScope::SYSTEM:
+    case SIAtomicScope::AGENT:
+      BuildMI(MBB, MI, DL, TII->get(AMDGPU::BUFFER_WBINVL1));
+      Changed = true;
+      break;
+    case SIAtomicScope::WORKGROUP:
+    case SIAtomicScope::WAVEFRONT:
+    case SIAtomicScope::SINGLETHREAD:
+      // No cache to invalidate.
+      break;
+    default:
+      llvm_unreachable("Unsupported synchronization scope");
+    }
+  }
+
+  /// The scratch address space does not need the global memory cache
+  /// to be flushed as all memory operations by the same thread are
+  /// sequentially consistent, and no other thread can access scratch
+  /// memory.
+
+  /// Other address spaces do not have a cache.
+
+  if (Pos == Position::AFTER)
+    --MI;
+
+  return Changed;
+}
+
 bool SIGfx6CacheControl::insertRelease(MachineBasicBlock::iterator &MI,
                                        SIAtomicScope Scope,
                                        SIAtomicAddrSpace AddrSpace,
                                        bool IsCrossAddrSpaceOrdering,
                                        Position Pos) const {
     return insertWait(MI, Scope, AddrSpace, SIMemOp::LOAD | SIMemOp::STORE,
                       IsCrossAddrSpaceOrdering, Pos);
 }
bool SIGfx10CacheControl::enableNonTemporal(
   bool Changed = false;
 
   Changed |= enableSLCBit(MI);
   /// TODO for store (non-rmw atomic) instructions also enableGLCBit(MI)
 
   return Changed;
 }
 
-bool SIGfx10CacheControl::insertAcquire(MachineBasicBlock::iterator &MI,
-                                        SIAtomicScope Scope,
-                                        SIAtomicAddrSpace AddrSpace,
-                                        Position Pos) const {
-  if (!InsertCacheInv)
-    return false;
-
-  bool Changed = false;
-
-  MachineBasicBlock &MBB = *MI->getParent();
-  DebugLoc DL = MI->getDebugLoc();
-
-  if (Pos == Position::AFTER)
-    ++MI;
-
-  if ((AddrSpace & SIAtomicAddrSpace::GLOBAL) != SIAtomicAddrSpace::NONE) {
-    switch (Scope) {
-    case SIAtomicScope::SYSTEM:
-    case SIAtomicScope::AGENT:
-      BuildMI(MBB, MI, DL, TII->get(AMDGPU::BUFFER_GL0_INV));
-      BuildMI(MBB, MI, DL, TII->get(AMDGPU::BUFFER_GL1_INV));
-      Changed = true;
-      break;
-    case SIAtomicScope::WORKGROUP:
-      // In WGP mode the waves of a work-group can be executing on either CU of
-      // the WGP. Therefore need to invalidate the L0 which is per CU. Otherwise
-      // in CU mode and all waves of a work-group are on the same CU, and so the
-      // L0 does not need to be invalidated.
-      if (!ST.isCuModeEnabled()) {
-        BuildMI(MBB, MI, DL, TII->get(AMDGPU::BUFFER_GL0_INV));
-        Changed = true;
-      }
-      break;
-    case SIAtomicScope::WAVEFRONT:
-    case SIAtomicScope::SINGLETHREAD:
-      // No cache to invalidate.
-      break;
-    default:
-      llvm_unreachable("Unsupported synchronization scope");
-    }
-  }
-
-  /// The scratch address space does not need the global memory cache
-  /// to be flushed as all memory operations by the same thread are
-  /// sequentially consistent, and no other thread can access scratch
-  /// memory.
-
-  /// Other address spaces do not have a cache.
-
-  if (Pos == Position::AFTER)
-    --MI;
-
-  return Changed;
-}
-
 bool SIGfx10CacheControl::insertWait(MachineBasicBlock::iterator &MI,
                                      SIAtomicScope Scope,
                                      SIAtomicAddrSpace AddrSpace,
                                      SIMemOp Op,
                                      bool IsCrossAddrSpaceOrdering,
                                      Position Pos) const {
   bool Changed = false;
 
bool SIGfx10CacheControl::insertWait(MachineBasicBlock::iterator &MI,
   }
 
   if (Pos == Position::AFTER)
     --MI;
 
   return Changed;
 }
 
+bool SIGfx10CacheControl::insertAcquire(MachineBasicBlock::iterator &MI,
+                                        SIAtomicScope Scope,
+                                        SIAtomicAddrSpace AddrSpace,
+                                        Position Pos) const {
+  if (!InsertCacheInv)
+    return false;
+
+  bool Changed = false;
+
+  MachineBasicBlock &MBB = *MI->getParent();
+  DebugLoc DL = MI->getDebugLoc();
+
+  if (Pos == Position::AFTER)
+    ++MI;
+
+  if ((AddrSpace & SIAtomicAddrSpace::GLOBAL) != SIAtomicAddrSpace::NONE) {
+    switch (Scope) {
+    case SIAtomicScope::SYSTEM:
+    case SIAtomicScope::AGENT:
+      BuildMI(MBB, MI, DL, TII->get(AMDGPU::BUFFER_GL0_INV));
+      BuildMI(MBB, MI, DL, TII->get(AMDGPU::BUFFER_GL1_INV));
+      Changed = true;
+      break;
+    case SIAtomicScope::WORKGROUP:
+      // In WGP mode the waves of a work-group can be executing on either CU of
+      // the WGP. Therefore need to invalidate the L0 which is per CU. Otherwise
+      // in CU mode and all waves of a work-group are on the same CU, and so the
+      // L0 does not need to be invalidated.
+      if (!ST.isCuModeEnabled()) {
+        BuildMI(MBB, MI, DL, TII->get(AMDGPU::BUFFER_GL0_INV));
+        Changed = true;
+      }
+      break;
+    case SIAtomicScope::WAVEFRONT:
+    case SIAtomicScope::SINGLETHREAD:
+      // No cache to invalidate.
+      break;
+    default:
+      llvm_unreachable("Unsupported synchronization scope");
+    }
+  }
+
+  /// The scratch address space does not need the global memory cache
+  /// to be flushed as all memory operations by the same thread are
+  /// sequentially consistent, and no other thread can access scratch
+  /// memory.
+
+  /// Other address spaces do not have a cache.
+
+  if (Pos == Position::AFTER)
+    --MI;
+
+  return Changed;
+}
+
 bool SIMemoryLegalizer::removeAtomicPseudoMIs() {
   if (AtomicPseudoMIs.empty())
     return false;
 
   for (auto &MI : AtomicPseudoMIs)
     MI->eraseFromParent();
 
   AtomicPseudoMIs.clear();