[Openmp-commits] [PATCH] D105073: [libomptarget] [amdgpu] Fix default setting of max flat workgroup size

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1922
   // check flat_max_work_group_size attr here
-  if (threadsPerGroup > ConstWGSize) {
+  if (ConstWGSize > 0 && threadsPerGroup > ConstWGSize) {
     threadsPerGroup = ConstWGSize;
----------------
I guess this avoids a case where threadsPerGroup is set to zero by a zero ConstWGSize.

We've had a few bugs in this area now. The function is a map from a few integers to a few integers containing a lot of domain knowledge about deriving good launch parameters. It's only tested from the OpenMP level, where we can't easily perturb these values, so we miss stuff.

I think it's time to move all the launch value manipulation logic into this function, add a boolean to disable printing, and exhaustively test that function at the unit level.

We can either do that by making the function constexpr and writing a bunch of static asserts in this file or by declaring the prototype in a header and adding a standalone file that runs a bunch of values through it. Either way, we'd end up with a clear list of the cases it's intended to handle and much better odds of eliminating bugs in the edge cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105073