[PATCH] D138443: [AMDGPU] Fix GCNSubtarget::getMinNumVGPRs, add unit test to check consistency between GCNSubtarget's getMinNumVGPRs, getMaxNumVGPRs and getOccupancyWithNumVGPRs.
Juan Manuel Martinez CaamaƱo via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 1 04:45:02 PST 2022
jmmartinez added inline comments.
================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:32
+std::unique_ptr<const GCNTargetMachine>
+llvm::createAMDGPUTargetMachine(std::string TStr, StringRef CPU, StringRef FS) {
+ InitializeAMDGPUTarget();
----------------
Maybe replace `std::string` by `StringRef` ?
================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:100
+
+ std::map<std::string, SmallVector<std::string>> TablePerCPUs;
+ for (auto CPUName : CPUs) {
----------------
foad wrote:
> 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>`?
> 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.
@vpykhtin What do you mean that it doesn't own the string data? When creating the `StringMapEntry` (see `StringMapEntryBase::allocateWithKey`) it performs a copy of the input `StringRef` into its own internal buffer.
The awkward side is that you'll have to use raw_string_ostream for building the `ErrStr` as shown below (notice the call to `.first()` instead of `.first`):
std::string ErrStr;
raw_string_ostream OS(ErrStr);
for (auto &P : TablePerCPUs) {
for (auto &CPUName : P.second)
OS << ' ' << CPUName;
OS << ":\nOcc Min" << RegName << " Max" << RegName << '\n'
<< P.first() << '\n';
}
EXPECT_TRUE(ErrStr.empty()) << ErrStr;
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