rampitec added inline comments.
================
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 {
----------------
t-tye wrote:
> 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?
It is probably better just print an error if amdhsa is requested but no
compatible -mcpu specified (e.g. when we have generic/tahiti etc).
================
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);
----------------
pvellien wrote:
> t-tye wrote:
> > pvellien wrote:
> > > 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?
> > In the early days of implementing HSA I believe we were bringing up on
> > gfx6. It could not support all HSA features, but it did function with the
> > parts it could support. So I was suggesting we restore the code to support
> > what it did originally. That would mean using MUBUF for the GLOBAL address
> > space like it used to do (is that code still present?).
> >
> > The compiler can then report errors for the features it cannot support,
> > which in this case is it cannot support instruction selection of the
> > GENERIC address space on gfx6.
> >
> > If you could find the commit that switched to using FLAT instructions to
> > access the GLOBAL address space that will likely provide the necessary
> > information to decide the best thing to do for this issue.
> I code to select MUBUF instructions for Global address space is still
> present, In fact my first patch for this issue is to generate MUBUF
> instructions instead of reporting error. But it got rejected due to SI ASICs
> do not support HSA.
> This is the patch [[ https://reviews.llvm.org/D15543 |
> https://reviews.llvm.org/D15543 ]] which switched to using FLAT instructions
> for global.
> So whether the new approach is to off FlatForGlobal flag for
> -mtriple=amdgcn-amd-amdhsa -mcpu=gfx60x combination and generate MUBUF
> instructions instead. It would be very much to know what is the expectation
> :) whether we wait for @rampitec for a comment
> Btw, thanks a lot for your feedback.
We can try to use MUBUF for global, but we certainly cannot support HSA and/or
generic pointers on SI.
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
