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

Fri Dec 11 11:42:52 PST 2020

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:
> > > > > 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.
> Actually there are bunch of lit tests which assumes Gen as CI if cpu isn't specified with AMDHSA OS. This change just means for HSA OS if the cpu isn't specified it initializes to CI, for non-HSA if cpu isn't specified it initializes to SI. 
> Do I want to change that  if cpu isn't specified, then all mtriple assumes to SI? 
> 
I still do not understand how does that correspond to the condition. That is the old condition:

```
TT.getOS() == Triple::AMDHSA ? SEA_ISLANDS : SOUTHERN_ISLANDS
```
And that is literally what you have described: "This change just means for HSA OS if the cpu isn't specified it initializes to CI, for non-HSA if cpu isn't specified it initializes to SI".

At the same time this code:

```
!GPU.contains("generic") ? SOUTHERN_ISLANDS
```
Reads: use SI for any CPU besides generic regardless of the triple.

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

https://reviews.llvm.org/D92483