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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:191
+    Gen(!GPU.contains("generic") ? SOUTHERN_ISLANDS :
+        (TT.getOS() == Triple::AMDHSA ? SEA_ISLANDS :
+    SOUTHERN_ISLANDS)),
----------------
rampitec wrote:
> Please at least remove the parenthesis and reformat this statement.
I think I finally understand what this is doing, but I still think it is worth trying this another way as it seems to have confused each of Stas, Tony, and I to some extent. I don't think this is Praveen's doing, the underlying mechanisms we use for this are just not very intuitive.

What I'd suggest is adding a new `INVALID = 0` variant to `enum AMDGPUSubtarget::Generation`, and changing this initializer to `Gen(INVALID)`.

Then in `GCNSubtarget::initializeSubtargetDependencies`, after the call to `ParseSubtargetFeatures`, check for `(Gen == INVALID)`, and implement whatever "generic" CPU stuff we want.

I'd also accompany both places (the `enum` and the code after `ParseSubtargetFeatures`) with a summary of the situation in a comment. Maybe:

```
  class AMDGPUSubtarget {
  public:
    enum Generation {
~     // INVALID is only used as an initial value before `ParseSubtargetFeatures`,
~     // and as a way to detect the "generic" processor case.
~     INVALID,
~     R600,
~     R700,
~     EVERGREEN,
~     NORTHERN_ISLANDS,
~     SOUTHERN_ISLANDS,
~     SEA_ISLANDS,
+     VOLCANIC_ISLANDS,
+     GFX9,
+     GFX10
    };
```

```
     ParseSubtargetFeatures(GPU, /*TuneCPU*/ GPU, FullFS);

+   // Implement the "generic" processors, which act as the default when no
+   // generation features are enabled (e.g. for -mcpu=''). There are two variants
+   // of the generic processor, one for HSA and one for everything else.
+   if (Gen == INVALID) {
+     Gen = TT.getOS() == Triple::AMDHSA ? SEA_ISLANDS : SOUTHERN_ISLANDS;
+   }
```

Thoughts? If this doesn't seem appreciably better to everyone I'm also OK with the patch as-is.


As a separate issue, which we can discuss and address in a separate patch if we choose to do anything, is why we need/want the "generic" processor to actually be two different processors, with the decision being based on the OS. Tony suggested, and I think I agree, that we should probably just pick one generation that is the "lowest common denominator" that supports the major things we need. As the flat address space is pretty fundamental why not just have this be `SEA_ISLANDS`? Then we just have:

```
if (Gen == INVALID) {
  Gen = SEA_ISLANDS;
}
```

Doing this breaks about a dozen lit tests, which I assume can all be fixed by either adding `-mcpu=gfx600` (i.e. they no longer test "generic") or updating some check lines.


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

https://reviews.llvm.org/D92483