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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
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)),
----------------
pvellien wrote:
> rampitec wrote:
> > pvellien wrote:
> > > rampitec wrote:
> > > > pvellien wrote:
> > > > > 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".
> > > > I would probably just error out if we have a generic CPU + amdhsa requested. It seems cleaner.
> > > Thanks for your comment, this patch preserves the earlier functionality except the use of mubuf. Perhaps we can address your suggestion in a separate patch with some added context, and exploring the possibilities of reporting the error in other target-triple, cpu pairs? 
> > It is OK to have it in a separate commit, but I do not follow logic here. You are setting SI for any NON-generic chip.
> For a non-generic cpu, the correct Gen member is set by the ParseSubtargetFeatures function call (generated by tablegen) in initializeSubtargetDependencies function. It is similar to the previous version of this code, where Gen is set to SI if OS is not HSA and set to CI if OS is HSA and correct Gen is set by the call to ParseSubtargetFeatures function. 
Then this line is a no-op? You say: "for a non-generic CPU gen is initialized elsewhere". An this change only forces SI for a non-generic CPU.


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

https://reviews.llvm.org/D92483