Hi Andrew,
Andrew Stubbs:
On 22/08/2024 19:26, Tobias Burnus wrote:
(A) Any comments, suggestions regarding the patch in general and in
particular the plugin/ related parts?
The code all looks pretty reasonable to me.
The header file conditional includes worry me though: it is adding
complexity in a way that hurts maintainability, and looks like it
might break somebody's hypothetical out-of-tree plugin. Is it not
better for a plugin that supports interop to include omp.h itself?
I do note that libgomp.h explicitly includes 'omp.h.in' – and later
includes 'libgomp-plugin.h' and not omp.h.
But I don't know why. It could be some build-related issue or because it
replaces already the locking definition by its own? (Albeit it could
still use 'omp.h' together with the current '#ifdef' protection.)
Assuming that omp.h.in is only included as the locking-type dance is
done – and not an actual build issues: I will try whether just including
'omp.h' in plugin/plugin-*.c and libgomp-plugin.c before
libgomp-plugin.h works. For libgomp.h, it is already included (and then
used by target.c).
* * *
(B) RFC: The *stream* *creation* (hsa_queue_t,
cudaStream_t/hipStream_t) functions have tons of options. Thus:
...
(ii) Should the user be able to tweak the values?
I mean, the user could say:** 'prefer_type({fr("cuda"),
attr("ompx_priority:-2,ompx_non_blocking")},{fr("hsa"),attr("ompx_queue_size:64"})'.
Do we want to permit this? If yes, which of the values should be
changeable?
Is there any prior art for this? It looks like it could be added in
future, without breaking backward compatibility, so I say "no" (at
least for now).
There is no real prior art as the 'attr' is a very new feature (voted in
in the about two months ago); I think it was mainly proposed for 'sycl'
to specify an 'in-order' queue, which is a commonly what needed, but the
default in sycl is an 'out-of-order' queue. In any case, it seems as if
they intent to provide either type of queue.
Still, if there is a sensible attribute to set, I think it makes sense
to actually add it – and 'ompx_gnu_' should avoid interoperability issues.
But as the feature is supported code wise, adding an attribute only
requires changing two files: The plugin-<type>.c and libgomp.texi, i.e.
that's simple and quick.
Tobias
