[Libomptarget] Remove extra cache for offloading entries #77012
Conversation
jhuber6
requested review from
dhruvachak,
jdoerfert,
JonChesterfield,
ronlieb and
shiltian
openmp/libomptarget/include/device.h
Outdated
| @@ -159,7 +159,7 @@ struct DeviceTy { | |||
| /// } | |||
|
|
|||
| /// Register \p Entry as an offload entry that is avalable on this device. | |||
| void addOffloadEntry(OffloadEntryTy &Entry); | |||
| void addOffloadEntry(OffloadEntryTy Entry); | |||
There was a problem hiding this comment.
It's only 2 ptrs now but why would we not want (const) references here?
There was a problem hiding this comment.
With the current usage, it's passed in as an r-value and then stored inside of some other struct. I wanted explicitly ownership by forcing the copy. It could probably be done another way.
There was a problem hiding this comment.
But it is live and unmodified during the call. I don't see why the copy helps. const ref seems better, especially since we should not store the reference as the entry could be invalid. A copy hides this, I'd say.
There was a problem hiding this comment.
It's fine as long as it's ultimately copied by the insert call, otherwise it's a reference to a temporary. I can make the change.
Summary: The offloading entries right now are assumed to be baked into the binary itself, and thus always valid whenever the library is executing. This means that we don't need to copy them to additional storage and can instead simply pass around references to it. This is not likely to change in the expected operation of the OpenMP library. Additionally, the indirection for the offload entry struct is simply two pointers, so moving it by value is trivial.
jhuber6
force-pushed
the
RemoveEntryCache
branch
from
51fcbf3 to
66028b5
Compare
There was a problem hiding this comment.
It looks like @jdoerfert 's comment has been resolved. LGTM.
Summary: The offloading entries right now are assumed to be baked into the binary itself, and thus always valid whenever the library is executing. This means that we don't need to copy them to additional storage and can instead simply pass around references to it. This is not likely to change in the expected operation of the OpenMP library. Additionally, the indirection for the offload entry struct is simply two pointers, so moving it by value is trivial.
Summary:
The offloading entries right now are assumed to be baked into the binary
itself, and thus always valid whenever the library is executing. This
means that we don't need to copy them to additional storage and can
instead simply pass around references to it.
This is not likely to change in the expected operation of the OpenMP
library. Additionally, the indirection for the offload entry struct is
simply two pointers, so moving it by value is trivial.