Closed lmoriche closed 5 months ago
@domcharrier, we're thinking we'll manually put this interface in, rather than using the binding generator. We should still be able to use the generator for updates once it's available.
I would benefit from your expertise on this review, if you have time to spare it.
I think
ret = roctxRangePushA(c_char_"hello_world"//c_null_char)
might be error prone (as users must append c_null_char
) but it is probably the only way
to pass a Fortran character array without allocating an extra buffer.
c_null_char
and/or do some character conversions would create a whole category of other issues as roctxRangePushA
internally does not create its own copy of the user markers but simply stores the char*
pointer. Therefore, the hipfort interface or the user code would need to keep the extra buffer alive as long as it is needed.This library will likely not grow dramatically in the future. So a handwritten version is fine. However, it can also be generated extremely easily. For now, keep it as it is, but you might prefer the automatically generated version in the future simply to keep track with documentation changes and the copyright year in the license text.
Your solution that asks the user to append c_null_char
is probably the most user-friendly that one can do without adding significant and possibly fragile infrastructure to manage temporary copies of input buffers.
A manually-written version is fine. In any case, it will be easy to replace it by an automatically generated version later at any time later on.
At a minimum, the function names should match the C API without the "A" suffixes.
The c_null_char
requirement should also be well documented in the ROCm documentation.
https://rocm.docs.amd.com/projects/hipfort/en/latest/reference/supported_apis.html
Not having users need to append c_null_char
would be much preferable. It surprises me that the roctx interfaces don't make copies internally. I would expect there to be lifetime issues if anyone tried to dynamically create a temporary strings to pass to the API.
@lmoriche @cgmb
Regarding @bcornille 's observation of the A
suffix.
I assume you chose that as the roctx
header file contains macros like the one below:
#define roctxMark(message) roctxMarkA(message)
where roctxMarkA
will be the name of the function symbol in the roctx
shared object.
Note that you can use the bind(c,name="...")
to your advantage here:
function roctxRangeStart(message) bind(c, name="roctxRangeStartA")
Note that you can use the
bind(c,name="...")
to your advantage here:function roctxRangeStart(message) bind(c, name="roctxRangeStartA")
Let's go with this. I'll apply those changes and we can get this merged.
Add bindings for the roctxMarkA, roctxRangePushA, roctxRangePop, roctxRangeStartA, and roctxRangeStop ROCTX API functions.
Add a rudimentary test in teat/f2003/roctx. The test pushes a ROCTX range around a kernel dispatch and device synchronization. The test uses rocprof to trace both HIP events and ROCTX events.