[Openmp-commits] [PATCH] D93727: [OpenMP] Add using bit flags to select Libomptarget Information

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added inline comments.


================
Comment at: openmp/libomptarget/include/Debug.h:49
+  // Print kernel information from target device plugins
+  OMP_INFOTYPE_PLUGIN_KERNEL = 0x0010,
+};
----------------
grokos wrote:
> Is 0x0008 reserved for something else?
No real reason, I just felt like keeping a space open, so if we add another libomptarget flag the first nibble just applies to libomptarget, since the fourth one is for the CUDA plugin right now.


================
Comment at: openmp/libomptarget/include/Debug.h:54
+  static uint32_t InfoLevel = 0;
+  static bool initialized = false;
+  if (initialized)
----------------
grokos wrote:
> I would suggest you declare `initialized` as `std::once_flag` and call `std::call_once()` to initialize `InfoLevel`. This is guaranteed to be thread-safe (unlike the implementation posted here) while also making sure that `InfoLevel` is initialized exactly once. We initialize RTLs in `RTLsTy::RegisterLib()` in the very same way.
Seems like a good solution. Should I push this first and address that in a future patch? Or just do it here.


================
Comment at: openmp/libomptarget/src/interface.cpp:237
   }
 
 #ifdef OMPTARGET_DEBUG
----------------
grokos wrote:
> Wouldn't we want to have
> ```
>   if (getInfoLevel() & OMP_INFOTYPE_KERNEL_ARGS)
>     printKernelArguments(loc, device_id, arg_num, arg_sizes, arg_types,
>                          arg_names);
> ```
> here as well? It is useful to see what arguments we have if something goes wrong at a `target exit data` directive or upon exiting a `target data` scope.
This method just prints the arguments to `__tgt_target_data_begin_mapper` and `__tgt_target_data_end_mapper`, which should have the same arguments right? I had it in previously but it didn't add any useful information when I tested it. I could add a separate message that just says something like "Exiting data region started at (loc)". Would that be good?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93727