[PATCH] D138443: [AMDGPU] Fix GCNSubtarget::getMinNumVGPRs, add unit test to check consistency between GCNSubtarget's getMinNumVGPRs, getMaxNumVGPRs and getOccupancyWithNumVGPRs.

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin marked 2 inline comments as done.
vpykhtin added inline comments.


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:28-30
+// implementation is in the llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
+std::unique_ptr<const GCNTargetMachine>
+createTargetMachine(std::string TStr, StringRef CPU, StringRef FS);
----------------
arsenm wrote:
> This is confusing. Probably should move this somewhere else and have a proper declaration for it. Also the name is too generic
I just reuse the function from the llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp, but this can be changed.


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:61
+
+  std::stringstream MinStr;
+  MinStr << (MinValid ? ' ' : '<') << ' ' << std::setw(3) << MinGPRs << " (O"
----------------
arsenm wrote:
> raw_string_ostream or raw_svector_ostream 
I have to work with std::string and std::ostream because of the unittest framework.


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:72-73
+bool hasW32(StringRef CanonicCPUName) {
+  return CanonicCPUName.starts_with("gfx10") ||
+         CanonicCPUName.starts_with("gfx11");
+}
----------------
arsenm wrote:
> Can you go just off the features? This is going to be annoying to maintain (I thought we already had a feature to indicate it supports wave32 vs. wave64 separate from the mode itself, but I don't see it)
The problem is that I cannot know the feature until I have GCNSubtarget ready, but then I have to recreate the machine. Maybe I can test if the GPU has w32 mode and then run it again with w64 mode.


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:86
+
+  std::map<std::string, SmallVector<std::string>> TablePerCPUs;
+  for (auto CPUName : CPUs) {
----------------
arsenm wrote:
> DenseMap
DenseMap doesn't work with std::string as a key.


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:110
+  }
+  std::stringstream OS;
+  for (auto &P : TablePerCPUs) {
----------------
arsenm wrote:
> I'm not sure why you're printing everything to a string before printing it. Why not just directly print if printing is enabled?
This is how unittest framework works. It's multitreaded and uses std::string and std::ostream everywhere. DenseMap doesn't work with std::string.

The actual output happens here as an indication of error:
```
118  EXPECT_TRUE(ErrStr.empty()) << ErrStr;
```
When the test pass it shouldn't print anything. I added "--print-cpu-reg-limits" command line option just to be able to print entire results if needed.


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:121
+
+TEST(AMDGPU, TestVGPRLimitsPerOccupancy) {
+  testGPRLimits("VGPR", true, [](std::stringstream &OS, unsigned Occ,
----------------
arsenm wrote:
> Can you also check SGPRs and LDS?
This is planned with subsequent patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138443