[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
llvm-commits at lists.llvm.org
Tue Nov 22 06:44:19 PST 2022
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
More information about the llvm-commits
mailing list