[offload][ompt] Added lookup function for device callbacks #110007
Conversation
|
@llvm/pr-subscribers-offload Author: Kaloyan Ignatov (kaloyan-ignatov) Changes
An open question is how to go about testing that the lookup function returns pointers to the actual placeholders. Is there an existing test where this can be added, or does a separate one need to be made? Full diff: https://github.com/llvm/llvm-project/pull/110007.diff 4 Files Affected:
diff --git a/offload/include/OpenMP/OMPT/Callback.h b/offload/include/OpenMP/OMPT/Callback.h
index 68cb43745eb1f8..afe54ea2ebb2c7 100644
--- a/offload/include/OpenMP/OMPT/Callback.h
+++ b/offload/include/OpenMP/OMPT/Callback.h
@@ -49,6 +49,8 @@ namespace omp {
namespace target {
namespace ompt {
+extern ompt_interface_fn_t ompt_device_fn_lookup(const char *s);
+
#define declareOmptCallback(Name, Type, Code) extern Name##_t Name##_fn;
FOREACH_OMPT_NOEMI_EVENT(declareOmptCallback)
FOREACH_OMPT_EMI_EVENT(declareOmptCallback)
diff --git a/offload/src/OpenMP/OMPT/Callback.cpp b/offload/src/OpenMP/OMPT/Callback.cpp
index ab0942ed4fd3f7..4da140f85025b3 100644
--- a/offload/src/OpenMP/OMPT/Callback.cpp
+++ b/offload/src/OpenMP/OMPT/Callback.cpp
@@ -27,6 +27,8 @@
#undef DEBUG_PREFIX
#define DEBUG_PREFIX "OMPT"
+using namespace llvm::omp::target::ompt;
+
// Define OMPT callback functions (bound to actual callbacks later on)
#define defineOmptCallback(Name, Type, Code) \
Name##_t llvm::omp::target::ompt::Name##_fn = nullptr;
@@ -34,7 +36,69 @@ FOREACH_OMPT_NOEMI_EVENT(defineOmptCallback)
FOREACH_OMPT_EMI_EVENT(defineOmptCallback)
#undef defineOmptCallback
-using namespace llvm::omp::target::ompt;
+int ompt_get_device_num_procs(ompt_device_t *device) { return 0; }
+
+ompt_device_time_t ompt_get_device_time(ompt_device_t *device) { return 0; }
+
+double ompt_translate_time(ompt_device_t *device, ompt_device_time_t time) {
+ return 0;
+}
+
+ompt_set_result_t ompt_set_trace_ompt(ompt_device_t *device,
+ unsigned int enable,
+ unsigned int etype) {
+ return ompt_set_error;
+}
+
+ompt_set_result_t ompt_set_trace_native(ompt_device_t *device, int enable,
+ int flags) {
+ return ompt_set_error;
+}
+
+int ompt_start_trace(ompt_device_t *device,
+ ompt_callback_buffer_request_t request,
+ ompt_callback_buffer_complete_t complete) {
+ return 0;
+}
+
+int ompt_pause_trace(ompt_device_t *device, int begin_pause) { return 0; }
+
+int ompt_flush_trace(ompt_device_t *device) { return 0; }
+
+int ompt_stop_trace(ompt_device_t *device) { return 0; }
+
+int ompt_advance_buffer_cursor(ompt_device_t *device, ompt_buffer_t *buffer,
+ size_t size, ompt_buffer_cursor_t current,
+ ompt_buffer_cursor_t *next) {
+ return 0;
+}
+
+ompt_record_t ompt_get_record_type(ompt_buffer_t *buffer,
+ ompt_buffer_cursor_t current) {
+ return ompt_record_ompt;
+}
+
+void *ompt_get_record_native(ompt_buffer_t *buffer,
+ ompt_buffer_cursor_t current,
+ ompt_id_t *host_op_id) {
+ return NULL;
+}
+
+ompt_record_abstract_t *ompt_get_record_abstract(void *native_record) {
+ return NULL;
+}
+
+ompt_interface_fn_t llvm::omp::target::ompt::ompt_device_fn_lookup(const char *s) {
+#define ompt_interface_fn(fn) \
+ fn##_t fn##_f = fn; \
+ if (strcmp(s, #fn) == 0) \
+ return (ompt_interface_fn_t)fn##_f;
+
+ FOREACH_OMPT_DEVICE_INQUIRY_FN(ompt_interface_fn)
+#undef ompt_interface_fn
+ return NULL;
+}
+
/// Forward declaration
class LibomptargetRtlFinalizer;
diff --git a/openmp/runtime/src/include/omp-tools.h.var b/openmp/runtime/src/include/omp-tools.h.var
index 471f46a9073ee7..92017c69ea244b 100644
--- a/openmp/runtime/src/include/omp-tools.h.var
+++ b/openmp/runtime/src/include/omp-tools.h.var
@@ -60,6 +60,23 @@
macro(ompt_get_target_info) \
macro(ompt_get_num_devices)
+#define FOREACH_OMPT_DEVICE_INQUIRY_FN(macro) \
+ macro (ompt_get_device_num_procs) \
+ macro (ompt_get_device_time) \
+ macro (ompt_translate_time) \
+ macro (ompt_set_trace_ompt) \
+ macro (ompt_set_trace_native) \
+ /* macro (ompt_get_buffer_limits) */ \
+ macro (ompt_start_trace) \
+ macro (ompt_pause_trace) \
+ macro (ompt_flush_trace) \
+ macro (ompt_stop_trace) \
+ macro (ompt_advance_buffer_cursor) \
+ macro (ompt_get_record_type) \
+ /* macro (ompt_get_record_ompt) */ \
+ macro (ompt_get_record_native) \
+ macro (ompt_get_record_abstract)
+
#define FOREACH_OMPT_STATE(macro) \
\
/* first available state */ \
diff --git a/openmp/runtime/src/ompt-general.cpp b/openmp/runtime/src/ompt-general.cpp
index 923eea2a563a91..fa8d8019d5ea13 100644
--- a/openmp/runtime/src/ompt-general.cpp
+++ b/openmp/runtime/src/ompt-general.cpp
@@ -121,6 +121,8 @@ static ompt_start_tool_result_t *libomptarget_ompt_result = NULL;
static ompt_interface_fn_t ompt_fn_lookup(const char *s);
+static ompt_interface_fn_t ompt_device_fn_lookup(const char *s);
+
OMPT_API_ROUTINE ompt_data_t *ompt_get_thread_data(void);
/*****************************************************************************
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -60,6 +60,23 @@ | |||
| macro(ompt_get_target_info) \ | |||
| macro(ompt_get_num_devices) | |||
|
|
|||
| #define FOREACH_OMPT_DEVICE_INQUIRY_FN(macro) \ | |||
There was a problem hiding this comment.
Unfortunately, clang-format does not format this file
There was a problem hiding this comment.
You can call it manually, but it might change a lot of unrelated stuff.
| // Define OMPT callback functions (bound to actual callbacks later on) | ||
| #define defineOmptCallback(Name, Type, Code) \ | ||
| Name##_t llvm::omp::target::ompt::Name##_fn = nullptr; | ||
| FOREACH_OMPT_NOEMI_EVENT(defineOmptCallback) | ||
| FOREACH_OMPT_EMI_EVENT(defineOmptCallback) | ||
| #undef defineOmptCallback | ||
|
|
||
| using namespace llvm::omp::target::ompt; | ||
| int ompt_get_device_num_procs(ompt_device_t *device) { return 0; } |
There was a problem hiding this comment.
According to clang-format this seems to be fine
kaloyan-ignatov
force-pushed
the
ompt-device-lookup
branch
from
e763658 to
a8df27c
Compare
|
@mhalk this one of the issues we talked about last week. I you have a better solution downstream, we can withdraw this PR |
kaloyan-ignatov
force-pushed
the
ompt-device-lookup
branch
from
a8df27c to
43b539f
Compare
…olders for not yet implemented device callbacks
kaloyan-ignatov
force-pushed
the
ompt-device-lookup
branch
from
43b539f to
7687817
Compare
| using namespace llvm::omp::target::ompt; | ||
| int ompt_get_device_num_procs(ompt_device_t *device) { return 0; } | ||
|
|
||
| ompt_device_time_t ompt_get_device_time(ompt_device_t *device) { return 0; } |
There was a problem hiding this comment.
Maybe use ompt_time_none instead? See 19.4.4.6 in the OpenMP 5.2 spec.
|
|
||
| ompt_set_result_t ompt_set_trace_ompt(ompt_device_t *device, | ||
| unsigned int enable, unsigned int etype) { | ||
| return ompt_set_error; |
There was a problem hiding this comment.
I would not choose ompt_set_error, as tools could think they did something wrong (e.g. tried to use target_emi and target at the same time, which is not allowed).
We should return ompt_set_never instead.
There was a problem hiding this comment.
ompt_set_error could still be used when trying to call this function outside of the ompt_callback_device_initialize callback, see the OpenMP 5.2 spec.
If the
ompt_set_trace_nativeor theompt_set_trace_omptruntime entry point is called outside a device initializer, registration of supported callbacks may fail with a return code ofompt_set_error.
|
|
||
| ompt_set_result_t ompt_set_trace_native(ompt_device_t *device, int enable, | ||
| int flags) { | ||
| return ompt_set_error; |
|
Aside from my comments, this looks mostly fine. It basically resembles what we were seeing with oneAPI until they removed the lookup stuff in 2024.1.0. However, this approach correctly returns that trying to register anything device tracing related will not work. I'm not sure if it makes sense to implement the lookup functions like |
@jprotze Thanks for the ping! Yes, we have another solution (esp. w.r.t. device tracing) downstream, which needs a bit of prepartation before actual upstreaming. We'd like to discuss further steps offline as this is beyond this PR and we'll reach out to you by mail. I think this might be a good opportunity to bring upstream device-tracing forward. |
| @@ -60,6 +60,23 @@ | |||
| macro(ompt_get_target_info) \ | |||
| macro(ompt_get_num_devices) | |||
|
|
|||
| #define FOREACH_OMPT_DEVICE_INQUIRY_FN(macro) \ | |||
There was a problem hiding this comment.
Downstream examples (e.g. here I like this location way better):
https://github.com/ROCm/llvm-project/blob/amd-staging/offload/include/OmptCommonDefs.h#L26-L46
| return 0; | ||
| } | ||
|
|
||
| ompt_set_result_t ompt_set_trace_ompt(ompt_device_t *device, |
There was a problem hiding this comment.
Downstream example (among others):
https://github.com/ROCm/llvm-project/blob/amd-staging/offload/plugins-nextgen/common/OMPT/OmptTracing.cpp#L120
| @@ -767,7 +767,7 @@ Error GenericDeviceTy::init(GenericPluginTy &Plugin) { | |||
| performOmptCallback(device_initialize, Plugin.getUserId(DeviceId), | |||
| /*type=*/getComputeUnitKind().c_str(), | |||
| /*device=*/reinterpret_cast<ompt_device_t *>(this), | |||
| /*lookup=*/ompt::lookupCallbackByName, | |||
| /*lookup=*/ompt::ompt_device_fn_lookup, | |||
There was a problem hiding this comment.
ompt_callback_device_initializewith a device-specific lookup functionAn open question is how to go about testing that the lookup function returns pointers to the actual placeholders. Is there an existing test where this can be added, or does a separate one need to be made?