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

> I think your patch shows that this documentation needs improving here. My understanding is:
> getMaxNumVGPRs returns the maximum number of VGPRs that you can use and still achieve at least the specified occupancy.
> getMinNumVGPRs returns the minimum number of VGPRs that will prevent you from achieving more than the specified occupancy.

Right, your description is precise, I'll change the documentation.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:629
+unsigned GCNSubtarget::getOccupancyWithNumVGPRs(unsigned NumVGPRs) const {
+  return AMDGPU::IsaInfo::getNumWavesWithNumVGPRs(this, NumVGPRs);
 }
----------------
foad wrote:
> Why change terminology from "Occupancy" to "NumWaves"? AMDGPUBaseInfo seems to use "WavesPerEU" a lot instead of just "NumWaves".
Do you think it would be more consistent to use WavesPerEU in AMDGPUBaseInfo? Like getNumWavesPerEUWithNumVGPRs?


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:998
+    return MaxWaves;
+  unsigned RoundedRegs = ((NumVGPRs + Granule - 1) / Granule) * Granule;
+  return std::min(std::max(getTotalNumVGPRs(STI) / RoundedRegs, 1u), MaxWaves);
----------------
foad wrote:
> This is `divideCeil(NumVGPRs, Granule) * Granule`. But how about adding `uint64_t alignUp(uint64_t Value, uint64_t Align)` to MathExtras.h (like `alignDown` but no need for the `Skew` parameter) and using it here?
I just didn't want to change the function body since I moved it but if I changed the name the body can be also imporved :)
MathExtras.h already has //alignTo//, I'll use it here.
```
inline uint64_t alignTo(uint64_t Value, uint64_t Align) {
  assert(Align != 0u && "Align can't be 0.");
  return (Value + Align - 1) / Align * Align;
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138443