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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield 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.
----------------
jhuber6 wrote:
> 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.
That is worth avoiding. The variable in the header doesn't achieve that though - each plugin will have it's own copy of the static variable + wrapper function.

If we move the init code, however it is done, into libomptarget then each plugin will need to make a cross-dl call to get the variable, but getenv is only called once. I think that's a win.

A comprehensively overengineered solution is to make the getenv calls once, from libomptarget, and also have the plugin read&cache the result of said call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93727