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

[praveen velliengiri via Phabricator via llvm-commits]

pvellien added a comment.

Comments inline



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:122-124
+  if (getGeneration() == AMDGPUSubtarget::SOUTHERN_ISLANDS) {
+    FlatForGlobal = false;
+    if (FS.contains("+flat-for-global"))
----------------
arsenm wrote:
> This isn't a generation check, and should be the same as the above addr64 check
I'm not sure I follow your comment, this patch uses MUBUF instructions only for gfx60x. The addr64 check returns true for both southern_island and sea_island processors. I think if we conditionally check only for hasAddr64, that may not be enough to handle the case where (FlatForGlobal = false, for SOUTHERN_ISLANDS and FlatForGlobal = true, for SEA_ISLANDS). 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:125
+    if (FS.contains("+flat-for-global"))
+       errs() << "GFX6 do not support Flat instructions for Global Address Space\n";
+  }
----------------
arsenm wrote:
> I'm not sure printing a warning here is particularly helpful. It's not going to be reported in any meaningful way. A proper Diagnostic would be better, but I don't think this would be the right place to emit it. 
I will remove this check and warning here.


================
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)),
----------------
t-tye wrote:
> Is this asking if the code contains use of the generic address space?
No, this isn't a check for generic address space. The GPU string is set to a string "generic" by default when no target cpu is specified. It has nothing to do with address spaces access.
when OS is HSA it is set to "generic-hsa" and in all other cases it is set to "generic".


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:193
+    Gen(!GPU.contains("generic") ? SOUTHERN_ISLANDS :
+        (TT.getOS() == Triple::AMDHSA ? SEA_ISLANDS : SOUTHERN_ISLANDS)),
+
----------------
t-tye wrote:
> This seems a very strange way for carrying this knowledge to the later code. Why not use a separate variable with a clear name?
In the previous version for this piece of code, the Gen member of this class is initialized to sea_islands when OS is HSA and southern_islands for all other cases and then the requested 'gen' is picked  by the tablegen generated function "ParseSubtargetFeatures" in initializeSubtargetDependencies. 

The above mentioned behavior is not correct, because it initializes the Gen is sea_islands for "-mtriple=amdgcn-amd-amdhsa -mcpu=gfx600" where the correct "Gen" would be southern islands. 

This piece of change fixes this bug, and also preserves the existing functionality. I don't think we need a separate variable since this is the only place we are handling. please correct me if I'm wrong. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92483