[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:93
   if (isAmdHsaOS()) // Turn on FlatForGlobal for HSA.
     FullFS += "+flat-for-global,+unaligned-access-mode,+trap-handler,";
 
----------------
scott.linder wrote:
> pvellien wrote:
> > t-tye wrote:
> > > pvellien wrote:
> > > > t-tye wrote:
> > > > > Seems +flat-for-global should not be added if SI. Suggest pulling up later logic to here and combine.
> > > > But the gen member variable is set to correct generation only after the call to parsesubtargetfeatures function, so it would be incorrect to use getGeneration function in this place.  I think it would be better to conditionally select later.
> > > But could that earlier code be using some other means to determine if the target supports flat operations? Is there no target property for that?
> > > 
> > > If not then leaving "as is" is fine with me. Just would be nice if the logic for each aspect was logically organized together rather than spread over different places making it hard to figure out what is happening.
> > As far as I know there is no target property for that. I  agree that it would be very much clear to have a common place for this mechanism
> There is `FeatureFlatAddressSpace`, but it will also only be set as part of `ParseSubtargetFeatures`.
> 
> I'm not sure why this needs to be here, unless some parts of `ParseSubtargetFeatures` depend on these features being set or not? Otherwise this could all be done below in one place.
I suppose the issue is that putting these after the call to `ParseSubtargetFeatures` makes allowing it to be overridden by the user messier?

It seems like it might be nice to just update TableGen to also maintain a bitmap of explicitly set features. Then we can avoid any string manipulation here (all of these things can move below the call, and can just make the default conditional on `ExplicitySet.test(FeatureFoo)`).

That way everyone has a uniform "escape hatch" for implementing things like mutual exclusion, or more complicated "implies" relationships, without all the string manipulation and comparison.


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

https://reviews.llvm.org/D92483