[PATCH] D47504: [AMDGPU] Simplify memory legalizer

[Tony Tye via Phabricator via llvm-commits]

t-tye marked 14 inline comments as done.
t-tye added inline comments.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:88
+  GDS = 1u << 3,
+  OTHER = 1u << 4,
+
----------------
rampitec wrote:
> Why do we need this OTHER?
It allows to determine if an instruction has only specific address spaces. To do that need to know it does not specify any other address spaces not explicitly modeled in atomics.


================
Comment at: lib/Target/AMDGPU/SIMemoryLegalizer.cpp:463
+
+  return SIAtomicAddrSpace::OTHER;
+}
----------------
rampitec wrote:
> Why not llvm_unreachable? Do you really expect this to happen?
I think these could happen for memory operations that have non-temporal.


================
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;
----------------
rampitec wrote:
> IsNonTemporal &= !MMO->isNonTemporal();
> 
> But what if you have two memory operands, one non-temporal and other is not?
Only treat the instruction as non-temporal if all MMOs specify non-temporal. An MMO that is not non-temporal indicates want to cache.


================
Comment at: test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir:3
+
+--- |
+  ; ModuleID = 'memory-legalizer-invalid-addrspace.ll'
----------------
rampitec wrote:
> Could you strip mir test from all IR part?
The MMO references the IR to get the address space so I believe it is needed.


https://reviews.llvm.org/D47504