[PATCH] D140264: [OpenMP] Improve AMDGPU Plugin

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:802
+      } else if (ActionFunction == releaseSignalAction) {
+        if (auto Err = releaseSignalAction(&ActionArgs))
+          return Err;
----------------
kevinsala wrote:
> 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.
The cast is a no-op, is it not? And the explicit one in your example is just implicitly done by the compiler in my code. `void* -> arg_ty *` is convertible.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:805
+      } else {
+        if (auto Err = (*ActionFunction)(&ActionArgs))
+          return Err;
----------------
kevinsala wrote:
> 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.
That works too. We can require to list them, won't be too many.


================
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)
----------------
kevinsala wrote:
> 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));
> ```
I don't follow completely. Can you make it a follow up?


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

https://reviews.llvm.org/D140264