[Libomptarget][NFC] Remove concept of optional plugin functions #82681
Conversation
Summary: Ever since the introduction of the new plugins we haven't exercised the concept of "optional" plugin functions. This is done in perparation for making the plugins use a static interface as it will greatly simplify the implementation if we assert that every function has the entrypoints. Currently some unsupported functions will just return failure or some other default value, so this shouldn't change anything.
jhuber6
requested review from
carlobertolli,
dhruvachak,
jdoerfert,
jplehr,
ronlieb,
saiislam and
shiltian
| PLUGIN_API_HANDLE(data_notify_unmapped); | ||
| PLUGIN_API_HANDLE(set_device_offset); | ||
| PLUGIN_API_HANDLE(initialize_record_replay); | ||
| PLUGIN_API_HANDLE(use_auto_zero_copy); |
There was a problem hiding this comment.
Hmm, what will happen on NVIDIA GPUs for use_auto_zero_copy?
There was a problem hiding this comment.
This patch doesn't change what happens if it's called, but the fallback is to just return false.
There was a problem hiding this comment.
I mean, for those APIs don't exist on a target, we will error out immediately with this change right? However, since they are optional, the program can and should still work even w/o them.
There was a problem hiding this comment.
No, we were never actually exercising this functionality. Every function is defined on every "plugin" with some returning default results, this change is NFC.
There was a problem hiding this comment.
Yeah now it's all virtual function and then have a unified implementation in PluginInterface.cpp. TBH I'm not sure if that's a good idea but that is out of the scope. For now it looks fine.
| PLUGIN_API_HANDLE(data_notify_unmapped); | ||
| PLUGIN_API_HANDLE(set_device_offset); | ||
| PLUGIN_API_HANDLE(initialize_record_replay); | ||
| PLUGIN_API_HANDLE(use_auto_zero_copy); |
There was a problem hiding this comment.
Yeah now it's all virtual function and then have a unified implementation in PluginInterface.cpp. TBH I'm not sure if that's a good idea but that is out of the scope. For now it looks fine.
|
In the future I want to move to a statically linked interface. Having the concept of "optional" functions is really difficult with a static interface, so we should just formalize it so some functions can just return "UNIMPLEMENTED" or something. |
|
@ronlieb @carlobertolli ASO version of this diff at https://gist.github.com/jhuber6/57e6b3867b158530bd44783491fc51a6. |
|
[AMD Official Use Only - General]
thanks
From: Joseph Huber ***@***.***>
Sent: Thursday, February 22, 2024 12:49 PM
To: llvm/llvm-project ***@***.***>
Cc: Lieberman, Ron ***@***.***>; Mention ***@***.***>
Subject: Re: [llvm/llvm-project] [Libomptarget][NFC] Remove concept of optional plugin functions (PR #82681)
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
@ronlieb<https://github.com/ronlieb> @carlobertolli<https://github.com/carlobertolli> ASO version of this diff at https://gist.github.com/jhuber6/57e6b3867b158530bd44783491fc51a6.
—
Reply to this email directly, view it on GitHub<#82681 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD3EYZ5JE4L4SKA3ZEW5BO3YU7DONAVCNFSM6AAAAABDVTAMWKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGQ3DGOJUGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
merges up to commit before 87b4108 [Libomptarget][NFC] Remove concept of optional plugin functions (llvm#82681) Change-Id: I1fc8cf975c675a254b29eb1beaf8e61bf3bef450
…#82681) Summary: Ever since the introduction of the new plugins we haven't exercised the concept of "optional" plugin functions. This is done in perparation for making the plugins use a static interface as it will greatly simplify the implementation if we assert that every function has the entrypoints. Currently some unsupported functions will just return failure or some other default value, so this shouldn't change anything. Change-Id: I6d635e18b80b849f031a6d7c81e8d93919e1645e
|
Are you planning to remove all the if(fptr) checks as well? Can be NFC now. |
Figured I'd just bundle that part in with switching over to static libraries. Looking at that currently. |
Summary:
Ever since the introduction of the new plugins we haven't exercised the
concept of "optional" plugin functions. This is done in perparation for
making the plugins use a static interface as it will greatly simplify
the implementation if we assert that every function has the entrypoints.
Currently some unsupported functions will just return failure or some
other default value, so this shouldn't change anything.