[PATCH] D92483: AMDGPU - Use MUBUF instructions for global address space access

[Tony Tye via Phabricator via llvm-commits]

t-tye requested changes to this revision.
t-tye added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:192
     TargetTriple(TT),
-    Gen(TT.getOS() == Triple::AMDHSA ? SEA_ISLANDS : SOUTHERN_ISLANDS),
+    Gen(!GPU.contains("generic") ? SOUTHERN_ISLANDS :
+        (TT.getOS() == Triple::AMDHSA ? SEA_ISLANDS : SOUTHERN_ISLANDS)),
----------------
Is this asking if the code contains use of the generic address space?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:193
+    Gen(!GPU.contains("generic") ? SOUTHERN_ISLANDS :
+        (TT.getOS() == Triple::AMDHSA ? SEA_ISLANDS : SOUTHERN_ISLANDS)),
+
----------------
This seems a very strange way for carrying this knowledge to the later code. Why not use a separate variable with a clear name?


================
Comment at: llvm/test/CodeGen/AMDGPU/si-global-buffer.ll:1-18
+; RUN: llc --mtriple=amdgcn-amd-amdhsa --mcpu=gfx600 -mattr=+flat-for-global -verify-machineinstrs <%s | FileCheck -check-prefix=SI %s
+; RUN: llc --mtriple=amdgcn  --mcpu=gfx600 -mattr=+flat-for-global -verify-machineinstrs <%s | FileCheck -check-prefix=SI %s
+
+define void @test(i32 addrspace(1)* %out){
+  ; SI-LABEL: test:
+  ; SI:       ; %bb.0:
+  ; SI-NEXT:  s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
----------------
I think it would be better to add a run line to existing memory-legalizer tests rather than have a separate test.

gfx600 now supports the memory model and so can now be tested by the memory-legalizer tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92483/new/

https://reviews.llvm.org/D92483