[Libomptarget] Fix data mapping on dynamic loads #80559
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: The current logic tries to map target mapping tables to the current device. Right now it assumes that data is only mapped a single time per device. This is only true if we have a single instance of the runtime running on a single program. However, in the case of dynamic library loads or shared libraries, this may happen multiple times. Given a case of a simple dynamic library load which has its own target kernel instruction, the current logic had only the first call to `__tgt_target_kernel` to the data mapping for that device. Then, when the next dynamic library load got called, it would see that the global were already mapped for that device and skip registering its own entires, even though they were distinct. This resulted in none of the mappings being done and hitting an assertion. This patch simply gets rid of this per-device check. The check should instead be on the host offloading entries. We already have logic that calls `continue` if we already have entries for that pointer, so we can simply rely on that instead.
jhuber6
requested review from
carlobertolli,
dhruvachak,
doru1004,
jdoerfert,
JonChesterfield,
jplehr,
ronlieb,
saiislam and
shiltian
ronlieb
approved these changes
Feb 3, 2024
agozillon
pushed a commit
to agozillon/llvm-project
that referenced
this pull request
Feb 5, 2024
Summary: The current logic tries to map target mapping tables to the current device. Right now it assumes that data is only mapped a single time per device. This is only true if we have a single instance of the runtime running on a single program. However, in the case of dynamic library loads or shared libraries, this may happen multiple times. Given a case of a simple dynamic library load which has its own target kernel instruction, the current logic had only the first call to `__tgt_target_kernel` to the data mapping for that device. Then, when the next dynamic library load got called, it would see that the global were already mapped for that device and skip registering its own entires, even though they were distinct. This resulted in none of the mappings being done and hitting an assertion. This patch simply gets rid of this per-device check. The check should instead be on the host offloading entries. We already have logic that calls `continue` if we already have entries for that pointer, so we can simply rely on that instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The current logic tries to map target mapping tables to the current
device. Right now it assumes that data is only mapped a single time per
device. This is only true if we have a single instance of the runtime
running on a single program. However, in the case of dynamic library
loads or shared libraries, this may happen multiple times.
Given a case of a simple dynamic library load which has its own target
kernel instruction, the current logic had only the first call to
__tgt_target_kernelto the data mapping for that device. Then, whenthe next dynamic library load got called, it would see that the global
were already mapped for that device and skip registering its own
entires, even though they were distinct. This resulted in none of the
mappings being done and hitting an assertion.
This patch simply gets rid of this per-device check. The check should
instead be on the host offloading entries. We already have logic that
calls
continueif we already have entries for that pointer, so we cansimply rely on that instead.