[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 added a comment.

> 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.

@foad I replaced call to getNumWavesPerEUWithNumVGPRs with a NumVGPRs value greater than getAddressableNumVGPRs with:

  if (MaxNumVGPRs == alignDown(TotNumVGPRs / MaxWavesPerEU, Granule))
    return 0;

but I cannot add assert now because it fails other tests at the moment.



================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:100
+
+  std::map<std::string, SmallVector<std::string>> TablePerCPUs;
+  for (auto CPUName : CPUs) {
----------------
jmmartinez wrote:
> 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;
This way you specify Value type as std::string but not the key.


================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:100
+
+  std::map<std::string, SmallVector<std::string>> TablePerCPUs;
+  for (auto CPUName : CPUs) {
----------------
vpykhtin wrote:
> jmmartinez wrote:
> > 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;
> This way you specify Value type as std::string but not the key.
> << P.first() << '\n';
I don't see () in my code.




================
Comment at: llvm/unittests/Target/AMDGPU/AMDGPUUnitTests.cpp:100
+
+  std::map<std::string, SmallVector<std::string>> TablePerCPUs;
+  for (auto CPUName : CPUs) {
----------------
vpykhtin wrote:
> vpykhtin wrote:
> > jmmartinez wrote:
> > > 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;
> > This way you specify Value type as std::string but not the key.
> > << P.first() << '\n';
> I don't see () in my code.
> 
> 
> When creating the StringMapEntry (see StringMapEntryBase::allocateWithKey) it performs a copy of the input StringRef into its own internal buffer

Ok, but here is the final argument: strings are sorted at the map and this gives nice picture at output :)  This map is only used on error or printing, it's not used during usual llvm-check routine so there is no penalty. 


================
Comment at: llvm/unittests/Target/AMDGPU/CMakeLists.txt:6-9
 set(LLVM_LINK_COMPONENTS
   AMDGPUCodeGen
   AMDGPUDesc
   AMDGPUInfo
----------------
jmmartinez wrote:
> I think `AMDGPUUtils` is missing from the list. Otherwise I get an undefined reference to `llvm::AMDGPU::IsaInfo::getMaxNumVGPRs(llvm::MCSubtargetInfo const*, unsigned int)` on my local build.
not sure, my build is ok, I build AMDGPUTests executable. Probably should go to another patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138443