[PATCH] D139468: AMDGPU: Clean up LDS-related occupancy calculations

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:146
+
+  if (AMDGPU::isGFX10Plus(*this) &&
+      !getFeatureBits().test(AMDGPU::FeatureCuMode))
----------------
nhaehnle wrote:
> arsenm wrote:
> > nhaehnle wrote:
> > > arsenm wrote:
> > > > I'd like to avoid growing the number of generation based feature checks in favor of separate subtarget features
> > > Hmm. I was using the same kind of check as what's already in AMDGPUBaseInfo. And it's not clear to me what the feature here would be that makes sense.
> > > 
> > > It is unfortunate that CuMode was picked out as the feature instead of WgpMode. Should we add CuMode as a feature to everything <gfx10? Or somehow figure out how to invert the sense of the feature, and make WgpMode the feature that has to be added explicitly? Or something else entirely?
> > I'd prefer inverting it but that's probably a good bit of migration pain.
> > 
> > The current feature is the dynamic switch; I was thinking another feature that says it has the dynamic switch. That's what we do for sramecc
> Would we still have a need for for the static feature if the dynamic switch was inverted? I see three options for this change right now:
> 
> 1. Commit is as-is.
> 2. First invert to feature WgpMode instead of feature CuMode.
> 3. Add a static feature SupportsWgpMode.
> 
> I believe option 2 is the best long-term path, but it feels like a pretty deep rabbit hole and I wouldn't want to make this change dependent on it. Option 3 could be done reasonably quickly, but it's not clear to me that it's actually better than option 1, if we're supposed to eventually invert anyway (at which point that static feature becomes unused?).
It's easy to remove the static feature after it's unused. It doesn't need to leak out into user IR in target-features attributes. I'm OK with this for now, up to you if you want to try for one of the better options


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139468