[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:41
+/// 31-Bit field data attributes controlling information presented to the user.
+enum OpenMPInfoType : int32_t {
+  // Print data arguments and attributes upon entering an OpenMP device kernel.
----------------
JonChesterfield wrote:
> jhuber6 wrote:
> > JonChesterfield wrote:
> > > Why not uint32, with zero for all flags clear as the default?
> > I wanted to have a method for getting the info / debug levels that didn't expose them as global variables, so I put them in a function as static variables. But this required having a special value for uninitialized so the value is only read from `getenv` the first time it's called. I could change it to use a boolean or something and avoid the complexity of reserving the highest order bit.
> How about code that runs on library load? We have a few getenv calls and it's probably good to do all of them once.
> 
> ```struct env {
> env() {all the getenv calls}
> 
> uint32_t debug_level() {return dl;}
> uint32_t info_level() {}
> 
> private:
>   uint32_t dl; // etc
> } environment;```
> 
> Makes the variables read-only after construction. Or use const member variables.
> 
> That will be thread safe, removes some branches, no sentinel. No teardown problems as destructor is a no-op.
This is basically how it was done previously, I didn't like needing to initialize it in every plugin separately when they're all sharing the same function. I could change it to work this way but I feel like the new approach using an extra boolean works alright.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93727