[PATCH] D47504: [AMDGPU] Simplify memory legalizer

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:55
+
+/// Memory operation flags. Can be ORed toether.
+enum class SIMemOp {
----------------
typo: together


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:63
+
+/// Position to instert a new instruction relative to an existing
+/// instruction.
----------------
typo: insert


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:88
+  GDS = 1u << 3,
+  OTHER = 1u << 4,
+
----------------
Why do we need this OTHER?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:103
+/// Sets named bit \p BitName to "true" if present in instruction \p
+/// MI.
+/// \returns Returns true if \p MI is modified, false otherwise.
----------------
Can you move this "MI" to the lane above?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:212
+  void reportUnsupported(const MachineBasicBlock::iterator &MI,
+                         const char *msg);
+
----------------
Please capitalize msg.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:214
+
+  /// Instepects the target symchonization scope \p SSID and
+  /// determines the SI atomic scope it corresponds to, the address
----------------
typos.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:219
+  Optional<std::tuple<SIAtomicScope, SIAtomicAddrSpace, bool>>
+  toSIAtomicScope(SyncScope::ID SSID, SIAtomicAddrSpace InstrScope);
+
----------------
const?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:222
+  /// \return Return a bit set of the address spaces accessed by \p
+  /// AS.
+  SIAtomicAddrSpace toSIAtomicAddrSpace(unsigned AS);
----------------
Please move AS to previous line. Line limit is 80 symbols.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:223
+  /// AS.
+  SIAtomicAddrSpace toSIAtomicAddrSpace(unsigned AS);
+
----------------
const


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:363
+
+  /// Return true iff instruction \p MI is a rmw atomic instruction
+  /// that returns a result.
----------------
I think rmw in the comment is misleading. It works with any atomic.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:419
+                               SIAtomicAddrSpace InstrScope) {
+  /// TODO For now assume OpenCL memory model which treats each
+  /// address space as having a separate happens-before relation, and
----------------
Can you use colon after TODO? Some of search this way in sources. Same below.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:463
+
+  return SIAtomicAddrSpace::OTHER;
+}
----------------
Why not llvm_unreachable? Do you really expect this to happen?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:484
   for (const auto &MMO : MI->memoperands()) {
-    const auto &IsSyncScopeInclusion =
-        MMI->isSyncScopeInclusion(SSID, MMO->getSyncScopeID());
-    if (!IsSyncScopeInclusion) {
-      reportUnknownSyncScope(MI);
-      return None;
-    }
-
-    SSID = IsSyncScopeInclusion.getValue() ? SSID : MMO->getSyncScopeID();
-    Ordering =
-        isStrongerThan(Ordering, MMO->getOrdering()) ?
-            Ordering : MMO->getOrdering();
-    FailureOrdering =
-        isStrongerThan(FailureOrdering, MMO->getFailureOrdering()) ?
-            FailureOrdering : MMO->getFailureOrdering();
-
     if (!(MMO->getFlags() & MachineMemOperand::MONonTemporal))
       IsNonTemporal = false;
----------------
IsNonTemporal &= !MMO->isNonTemporal();

But what if you have two memory operands, one non-temporal and other is not?


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:947
+      /// ordering and memory scope, then library does not need to
+      /// generate a fence. Could hadd support in this file for
+      /// barrier. SIInsertWaitcnt.cpp could then stop unconditionally
----------------
typo: add


================
Comment at: test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir:1
+# RUN: not llc -march=amdgcn -mcpu=gfx803 -run-pass si-memory-legalizer %s -o - 2>&1 | FileCheck %s
+
----------------
Please use -check-prefix=GCN


================
Comment at: test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir:3
+
+--- |
+  ; ModuleID = 'memory-legalizer-invalid-addrspace.ll'
----------------
Could you strip mir test from all IR part?


================
Comment at: test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir:69
+name:            invalid_load
+alignment:       0
+exposesReturnsTwice: false
----------------
All these attributes are also not needed in the test.


https://reviews.llvm.org/D47504