[PATCH] D140264: [OpenMP] Improve AMDGPU Plugin

[Kevin Sala Penadés via Phabricator via llvm-commits]

kevinsala added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h:69
   unsigned GV_Max_Teams;
+  // The default number teams
+  unsigned GV_Default_Num_Teams;
----------------
nit


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:802
+      } else if (ActionFunction == releaseSignalAction) {
+        if (auto Err = releaseSignalAction(&ActionArgs))
+          return Err;
----------------
We could pass a specialized pointer to the arguments too:

```
if (auto Err = releaseSignalAction((ReleaseSignalArgsTy *) &ActionArgs))
  return Err;
```

This would remove the cast on all action functions.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:805
+      } else {
+        if (auto Err = (*ActionFunction)(&ActionArgs))
+          return Err;
----------------
I like this approach. But since we don't allow yet other actions than the pre-defined ones, calling unknown actions may hide future errors. I would return error if the action is not recognized.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1546
+
     GridValues.GV_Max_Teams = GridMaxDim.x / GridValues.GV_Max_WG_Size;
     if (GridValues.GV_Max_Teams == 0)
----------------
I think here we're still overwriting the value of max teams determined by `OMP_NUM_TEAMS` in the `GenericDeviceTy` constructor. The same for `GV_Max_WG_Size` and  `OMP_TEAMS_THREAD_LIMIT`. To fix it, we could move the code below from the `GenericDeviceTy` constructor to the `GenericDeviceTy::init`:


```
Error GenericDeviceTy::init(GenericPluginTy &Plugin) {
  // NOTE: This call will initialize GV_Max_Teams and GV_Max_WG_Size with device's maximum values.
  if (auto Err = initImpl(Plugin))
    return Err;

  // Moved code. Originally in GenericDeviceTy constructor.
  if (OMP_NumTeams > 0)
    GridValues.GV_Max_Teams =
        std::min(GridValues.GV_Max_Teams, uint32_t(OMP_NumTeams));

  if (OMP_TeamsThreadLimit > 0)
    GridValues.GV_Max_WG_Size =
        std::min(GridValues.GV_Max_WG_Size, uint32_t(OMP_TeamsThreadLimit));
```


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

https://reviews.llvm.org/D140264