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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:146
+
+  if (AMDGPU::isGFX10Plus(*this) &&
+      !getFeatureBits().test(AMDGPU::FeatureCuMode))
----------------
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?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139468