This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][Libomptarget] Introduce changes to support remote plugin
ClosedPublic

Authored by atmnpatel on Dec 15 2020, 4:04 AM.

Details

Summary

In order to support remote execution, we need to be able to send the
target binary description to the remote host for registration (and
consequent deregistration). To support this, I added these two
optional new functions to the plugin API:

  • __tgt_rtl_register_lib
  • __tgt_rtl_unregister_lib

These functions will be called to properly manage the instance of
libomptarget running on the remote host.

Diff Detail

Event Timeline

atmnpatel created this revision.Dec 15 2020, 4:04 AM
atmnpatel requested review of this revision.Dec 15 2020, 4:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 4:04 AM
atmnpatel edited the summary of this revision. (Show Details)Dec 15 2020, 4:05 AM

Comments inline. I'd like to use the same hook to fix the memory manager lifetime error

openmp/libomptarget/src/interface.cpp
99

I suspect we should call loadrtls from the libomptarget init but that could be a different patch

116

Not sure whether the new calls should be before or after the existing one, but the order should be reversed for unregister relative to register to nest the lifetimes

openmp/libomptarget/src/rtl.cpp
26

Goes at the end, precedence given to first to ship

271

Moving this out of registerlib looks great

openmp/libomptarget/src/rtl.h
56

Functions should return non-void so they can indicate failure. Also, prefer function pointer to function type for clarity - you're missing an asterisk

atmnpatel updated this revision to Diff 311873.EditedDec 15 2020, 4:58 AM

Addresses inline comments (partially, fixing).

atmnpatel updated this revision to Diff 311875.Dec 15 2020, 5:10 AM

Added debug messages for when the new calls fail.

atmnpatel added inline comments.Dec 15 2020, 5:21 AM
openmp/libomptarget/src/interface.cpp
116

I kept it in this order because the remote plugin implementation receives __tgt_bin_desc(s) and uses that call to transfer the images/offload entries exactly once, and since RegisterLib asks if images are valid against RTLs, I needed to have libomptarget send the device images first, but if there's a compelling reason to swap the order, I'm happy to swap it.

118

I'm assuming we can get away with just outputting a debug message for when the calls fail? I don't think its right to abort...

openmp/libomptarget/src/interface.cpp
116

Pattern is usually:

  • generic construction
  • specific construction
  • use the objects
  • specific destruction
  • generic destruction

Lines up with base/derived patterns and lexical scoping of lifetimes.

If swapping generic + specific makes more sense here (which it might) then cool. The order of destruction should definitely be the opposite of the order of construction though.

JonChesterfield accepted this revision.Dec 15 2020, 7:22 AM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 15 2020, 7:22 AM
atmnpatel updated this revision to Diff 312347.Dec 16 2020, 5:33 PM

More fixes.

atmnpatel updated this revision to Diff 318763.Jan 23 2021, 9:13 AM

Removed redundant third function register_requires, it may become necessary later but it isn't now.

atmnpatel edited the summary of this revision. (Show Details)Jan 23 2021, 9:14 AM
This revision was landed with ongoing or failed builds.Jan 26 2021, 11:21 AM
This revision was automatically updated to reflect the committed changes.

I don't understand when/why an RTLsTy object is copied at all. @atmnpatel What if we move LoadRTLs into a helper function that is not a member, pass a pointer into it instead. That might solve the issue. Or, pass &PM.RTLs instead, to match the former call_once call. Can you take a look?