[PATCH] D83674: [AMDGPU] Calculate minimum allowed occupancy based on threads per lane

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

nhaehnle added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:919
 
-  unsigned getMinAllowedOccupancy() const {
-    if (!isMemoryBound() && !needsWaveLimiter())
-      return Occupancy;
-    return (Occupancy < 4) ? Occupancy : 4;
-  }
+  unsigned getMinAllowedOccupancy(const GCNSubtarget &ST) const;
 
----------------
arsenm wrote:
> nhaehnle wrote:
> > arsenm wrote:
> > > nhaehnle wrote:
> > > > The `ST` argument requirement seems annoying. Wouldn't it make sense for the MachineFunctionInfo to know the underlying MachineFunction, which would allow access to the GCNSubtarget (i.e. most-derived TargetSubtargetInfo)?
> > > No, I'm actively trying to remove references to the machine function state from MachineFunctionInfo
> > Why is that? MachineFunctionInfo is explicitly created with the MachineFunction as a constructor argument?
> That's what I'm trying to fix in D80249
I see. I commented on that change because I don't think it's quite right, but regardless of the outcome of that discussion: D80249 still gives the MFI accesses to the TargetSubtargetInfo, which is what we're talking about here. Even with that change, passing the ST yet again into this method should be unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83674