[PATCH] D138443: [AMDGPU] Fix GCNSubtarget::getMinNumVGPRs, add unit test to check consistency between GCNSubtarget's getMinNumVGPRs, getMaxNumVGPRs and getOccupancyWithNumVGPRs.
Jay Foad via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 1 02:57:44 PST 2022
foad added a comment.
I have no objection to committing this.
The only thing I'd like to try cleaning up in future is that getMinNumVGPRs relies on calling getNumWavesPerEUWithNumVGPRs with a NumVGPRs value greater than getAddressableNumVGPRs and still getting sensible answers, which seems to me like a case that getNumWavesPerEUWithNumVGPRs should not have to handle. I.e. I think we should be able to do this:
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
@@ -1020,6 +1020,7 @@ unsigned getAddressableNumVGPRs(const MCSubtargetInfo *STI) {
unsigned getNumWavesWithNumVGPRs(const MCSubtargetInfo *STI,
unsigned NumVGPRs) {
+ assert(NumVGPRs <= getAddressableNumVGPRs(STI));
NumVGPRs = std::min(std::max(NumVGPRs, 1u), getAddressableNumVGPRs(STI));
return std::min(getTotalNumVGPRs(STI) /
unsigned(alignTo(NumVGPRs, getVGPRAllocGranule(STI))),
but currently we can't.
================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:100
+
+ std::map<std::string, SmallVector<std::string>> TablePerCPUs;
+ for (auto CPUName : CPUs) {
----------------
vpykhtin wrote:
> arsenm wrote:
> > vpykhtin wrote:
> > > arsenm wrote:
> > > > StringMap?
> > > I need std::string as a key, StringMap uses StringRef but it doesn't own string data. Its not a big deal here because its only used on test fail or when printing.
> > DenseMap
> DenseMap doesn't work with std::string, it's not specialized for it.
`StringMap<std:string>`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138443/new/
https://reviews.llvm.org/D138443
More information about the llvm-commits
mailing list