[PATCH] D92115: AMDGPU - Add diagnostic for compiling modules with AMD HSA OS type and GFX 6 arch

[praveen velliengiri via Phabricator via llvm-commits]

pvellien added inline comments.


================
Comment at: llvm/docs/AMDGPUUsage.rst:2109-2112
+Caution:
+  AMD HSA Os is not supported in Southern Islands (GFX6) ASICs.
+
 For example:
----------------
t-tye wrote:
> 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.
Thanks for your feedback, I will update this


================
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);
----------------
t-tye wrote:
> 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.
Previously in the internal review process it mentioned that gfx60x does not support HSA and agreed to add a diagnostic to report that GFX6 do not support HSA OS type, @rampitec mentioned that SI ASICs cannot support HSA because we can't able to map memory on SI as HSA requires so the user will just have weird runtime failures. But based on your comment it seems like we have to use MUBUF instructions for -mtriple=amdgcn-amd-amdhsa -mcpu=gfx60x combination and use FLAT instructions for -mtriple=amdgcn-amd-amdhsa -mcpu=gfx70x+. Is my understanding correct? If the compiler emits the MUBUF instructions for global address space accesses, it is still required to produce the error msg? 


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

https://reviews.llvm.org/D92115