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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I thought I noticed funny behavior of the min occupancy while working on D115559 <https://reviews.llvm.org/D115559>



================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:20
+#include "gtest/gtest.h"
+#include <thread>
+
----------------
Not sure why you need this 


================
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);
----------------
This is confusing. Probably should move this somewhere else and have a proper declaration for it. Also the name is too generic


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:32
+
+bool checkMinMax(std::stringstream &OS, unsigned Occ, unsigned MinOcc,
+                 unsigned MaxOcc, std::function<unsigned(unsigned)> GetOcc,
----------------
static 


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:61
+
+  std::stringstream MinStr;
+  MinStr << (MinValid ? ' ' : '<') << ' ' << std::setw(3) << MinGPRs << " (O"
----------------
raw_string_ostream or raw_svector_ostream 


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:71
+
+bool hasW32(StringRef CanonicCPUName) {
+  return CanonicCPUName.starts_with("gfx10") ||
----------------
static


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:72-73
+bool hasW32(StringRef CanonicCPUName) {
+  return CanonicCPUName.starts_with("gfx10") ||
+         CanonicCPUName.starts_with("gfx11");
+}
----------------
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)


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:76-78
+static std::vector<std::pair<StringRef, StringRef>>
+    EmptySet = {{"", ""}},
+    W32W64 = {{"+wavefrontsize32", "w32"}, {"+wavefrontsize64", "w64"}};
----------------
static const std::array?


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:80
+
+void testGPRLimits(
+    const char *RegName, bool TestW32W64,
----------------
static


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:86
+
+  std::map<std::string, SmallVector<std::string>> TablePerCPUs;
+  for (auto CPUName : CPUs) {
----------------
DenseMap


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:98
+                      std::string(TM->getTargetFeatureString()), *TM);
+      std::stringstream Table;
+      bool Success = true;
----------------
Same as above


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:110
+  }
+  std::stringstream OS;
+  for (auto &P : TablePerCPUs) {
----------------
I'm not sure why you're printing everything to a string before printing it. Why not just directly print if printing is enabled?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138443