Libomptargert currently relies on global variables and this prevents
reusability of the functions for device plugins that may want to offload
themselves. This patch refactors the existing libomptarget code into an `OffloadingPlugin`
class. The `OffloadingPlugin` class just encapsulates the variables containing
all of the devices infomation and runtime plugins information.
Details
- Reviewers
jdoerfert ABataev JonChesterfield
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
350 ms | windows > LLVM.tools/llvm-cov::warnings.h |
Event Timeline
Getting rid of global state is a worthy goal. I think you've found all the global state, put it in a class, moved the functions acting on it into methods, then instantiated that class once to get a single object that is passed into each function.
Grouping all the state in a class that is instantiated once and passed in is good. I think moving all the functions into members that act on that one instance is a step too far, especially in the same patch. Having the functions callable from a C API is a good feature.
You could make a class instance with public fields that is passed to each of the C functions (by pointer), put all the global state in that class, and stay with free functions. No member functions. Probably not even a constructor, as we might want to handle failing to construct. That'll be functionally equivalent to the above and a much smaller patch to review.
If the consensus is that we we want the class methods instead of free functions, that can be a follow up patch that moves free functions into the class, which will be easier to read after the above.
openmp/libomptarget/src/rtl.cpp | ||
---|---|---|
25 | If construction fails, we presumably call std::terminate here? |
Thank you for the comments. I think I follow, I'll post another less intrusive patch soon.
clang-tidy: warning: invalid case style for function 'omp_get_num_devices' [readability-identifier-naming]
not useful