Page MenuHomePhabricator

[WIP][libomptarget][NFC] Refactor Libomptarget functionality into a class

Authored by atmnpatel on Oct 18 2020, 11:54 AM.



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.

Diff Detail

Unit TestsFailed

350 mswindows >
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-cov.exe show C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/prevent_false_instantiations.covmapping -instr-profile C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov\warnings.h -allow-empty -check-prefix=FAKE-FILE-STDOUT

Event Timeline

atmnpatel created this revision.Oct 18 2020, 11:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2020, 11:54 AM
atmnpatel requested review of this revision.Oct 18 2020, 11:54 AM

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.


If construction fails, we presumably call std::terminate here?

atmnpatel abandoned this revision.Oct 29 2020, 8:22 PM

Thank you for the comments. I think I follow, I'll post another less intrusive patch soon.