Page MenuHomePhabricator

[libomptarget] Load images in order of registration
ClosedPublic

Authored by manorom on Jan 27 2021, 7:49 AM.

Details

Summary

This makes sure that images are loaded in the order in which they are registered with libomptarget.

If a target can load multiple images and these images depend on each other (for example if one image contains the programs target regions and one image contains library code), then the order in which images are loaded can be important for symbol resolution (for example, in the VE plugin).
In this case: because the same code exist in the host binaries, the order in which the host linker loads them (which is also the order in which images are registered with libomptarget) is the order in which the images have to be loaded onto the device.

Diff Detail

Event Timeline

manorom requested review of this revision.Jan 27 2021, 7:49 AM
manorom created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 7:49 AM
manorom added a subscriber: tcramer.
simoll added inline comments.Jan 28 2021, 12:33 AM
openmp/libomptarget/src/rtl.cpp
151–155

Can we make this a callback instead? Eg,

*((void **)&R.supports_empty_images) =
     dlsym(dynlib_handle, "__tgt_rtl_supports_empty_images");
manorom updated this revision to Diff 321048.Feb 3 2021, 3:59 AM
  • Use callback to test for support for empty images
manorom marked an inline comment as done.Feb 3 2021, 4:00 AM
JonChesterfield accepted this revision.Feb 3 2021, 8:44 AM

There are two changes here. Loading images in a given order and loading images even if they look empty.

I'd personally split this, because annoying though it is to split a change, the whole thing is likely to be reverted if part of it turns out to be faulty.

Should fix the formatting nit above before landing. git-clang-format is great for that.

openmp/libomptarget/src/rtl.h
57

This is weird. Most of these typedefs are for function types, as opposed to the corresponding function pointer type. I guess matching the existing ones is right, but I think we're missing a '*' in most of these.

This revision is now accepted and ready to land.Feb 3 2021, 8:44 AM
tianshilei1992 added inline comments.Feb 3 2021, 9:10 AM
openmp/libomptarget/src/rtl.h
57

It starts from register_lib_ty and unregister_lib. :-)

manorom updated this revision to Diff 322634.Feb 10 2021, 3:05 AM
  • Fix formatting

I'd personally split this, because annoying though it is to split a change, the whole thing is likely to be reverted if part of it turns out to be faulty.

Since you already accepted this patch, I guess it is ok if we leave it in one patch? If the whole thing has to be reverted it is ok for us. In that case we would fix it with an additonal / splitted patch.

Yep, fine to commit as one patch. Recommendation was to reduce the amount of rework if this breaks something and gets reverted, not a requirement.

This revision was landed with ongoing or failed builds.Feb 24 2021, 9:15 AM
This revision was automatically updated to reflect the committed changes.