t-tye requested changes to this revision.
t-tye added inline comments.
This revision now requires changes to proceed.
================
Comment at: llvm/docs/AMDGPUUsage.rst:2109-2112
+Caution:
+ AMD HSA Os is not supported in Southern Islands (GFX6) ASICs.
+
For example:
----------------
This is not the right place for mentioning this. The Processor table would
likely be a better place. It should be in terms of supporting the amdhsa ABI.
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:62-72
+static AMDGPUSubtarget::Generation initializeGen(const Triple &TT,
+ StringRef GPU) {
+ if (GPU.contains("generic")) {
+ return TT.getOS() == Triple::AMDHSA
+ ? AMDGPUSubtarget::Generation::SEA_ISLANDS
+ : AMDGPUSubtarget::Generation::SOUTHERN_ISLANDS;
+ } else {
----------------
I am not clear what this function is doing. It seems to be returning a
generation unrelated to to the actual target generation to satisfy the one
place it is called. If the target is not SEA_ISLANDS it seems incorrect to be
returning SEA_ISLANDS. If this function is doing something special for the one
place it is called perhaps it should be expanded there?
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:134
+ if (isAmdHsaOS() && getGeneration() == AMDGPUSubtarget::SOUTHERN_ISLANDS) {
+ report_fatal_error("GFX6 (SI) ASICs does not support AMD HSA OS type \n",
+ false);
----------------
rampitec wrote:
> "do not support". I would also drop "(SI)" from the message. Maybe even
> better just "GFX6 does not support AMD HSA".
Make the message include the full target triple text so the user understands
how to resolve the issue. For example:
The target triple %s is not supported: the processor %s does not support the
amdhsa OS
Do the r600 targets also produce a similar error message?
Is this really the right test? My understanding is that the issue is not that
gfx60x does not support the amdhsa OS, but that it does not use the FLAT
address space.
My understanding is that the current problem is that FLAT instructions are
being used for the GLOBAL address space accesses. The use of FLAT instructions
for the global address space was introduced after gfx60x was initially being
supported on amdhsa. Originally BUFFER instructions that use an SRD that has a
0 base and are marked as addr64 where used for GLOBAL address space accesses.
This was changed to use FLAT instructions due to later targets dropping the SRD
addr64 support. I suspect it is that change that broke gfx60x as there were no
tests to catch it.
So the real fix seems to find that change and make the code still use use
BUFFER instructions for gfxx60x and FLAT instructions for gfx70x+. The tests
can then be updated to test gfx60x for amdhsa but to omit the FLAT address
space tests. The error would then indicate that the gfx60x does not support the
FLAT address space (and that is not conditional on the OS). The documentation
in AMDGPUUsage can state that gfx60x does not support the FLAT address space in
the Address Space section. The Processor table can add a column for processor
characteristics and mention that the gfx60x targets do not support the FLAT
address space.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92115/new/
https://reviews.llvm.org/D92115
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
