On Tue, Mar 18, 2025 at 03:36:58PM +0100, Paul-Antoine Arras wrote:
> This patch partially enables use of the OpenMP interop construct by adding
> middle end support, mostly in the omplower pass, and in the target-independent
> part of the libgomp runtime. It follows up on previous patches for C, C++ and
> Fortran front ends support. The full interop feature requires another patch to
> enable foreign runtime support in libgomp plugins.
Deferring most of review to Tobias, just nits from my side:
> +/* Generate code to implement the action-clauses (destroy, init, use) of an
> + OpenMP interop construct. */
> +
> +static void
> +lower_omp_interop_action_clauses (gimple_seq *seq, vec<tree> &objs,
> + vec<tree> *interop_types = NULL,
> + vec<tree> *prefer_types = NULL)
...
> + /* If there is only one object then we pass a single pointer,
> + if there are multiple objects then we build an array. */
> + if (objs.length () == 1)
> + {
> + tree obj = OMP_CLAUSE_DECL (objs.pop ());
> + if (TREE_CODE (TREE_TYPE (obj)) == REFERENCE_TYPE)
> + obj = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (obj)), obj);
> + if (action != OMP_CLAUSE_USE
> + && TREE_CODE (TREE_TYPE (obj)) != POINTER_TYPE)
> + {
> + // For modifying actions, we need a pointer.
Please don't mix comment styles, if /* comments are common in some source
file, just use them everywhere.
> + tree obj_ptr = create_tmp_var (build_pointer_type (TREE_TYPE (obj)));
> + gimplify_assign (obj_ptr, build_fold_addr_expr (obj), seq);
> + obj = obj_ptr;
> + }
> + else if (action == OMP_CLAUSE_USE
> + && TREE_CODE (TREE_TYPE (obj)) == POINTER_TYPE)
> + {
> + // For use action, we need the value.
Ditto.
> + tree prefer_type = prefer_types->pop ();
> + tree pref = prefer_type == NULL_TREE
> + ? null_pointer_node
> + : build_fold_addr_expr (prefer_type);
I think Emacs users would like ()s around here, so
tree pref = (prefer_type == NULL_TREE
? null_pointer_node
: build_fold_addr_expr (prefer_type));
> + gcc_assert (objs.is_empty () && (!interop_types || interop_types->is_empty
> ())
> + && (!prefer_types || prefer_types->is_empty ()));
The rule is if some &&/|| using condition fits on one line, it is on
one line, if it doesn't, each && or || operand goes on its own.
So
gcc_assert (objs.is_empty ()
&& (!interop_types || interop_types->is_empty ())
&& (!prefer_types || prefer_types->is_empty ()));
here.
> + /* Emit call to GOMP_interop:
> + void
> + GOMP_interop (int device_num, int n_init, void *init,
> + const void *target_targetsync, const void *prefer_type,
> + int n_use, void *use, int n_destroy, void *destroy,
> + unsigned int nowait, void **depend) */
Is nowait just 0/1? I wonder if it wouldn't be better to have the argument
called flags and accept a bitmask to make it more extensible for the
future (like we have unsigned int flags on GOMP_target_ext or on GOMP_task).
> + target_targetsync.safe_push (
> + build_int_cst (integer_type_node, target_targetsync_bits));
This kind of formatting is too ugly and can be easily avoided.
Just do
tree t = build_int_cst (integer_type_node, target_targetsync_bits);
target_targetsync.safe_push (t);
> + gcall *call = gimple_build_call (
> + fn, 11, device_num, n_init,
> + init_objs.length () ? init_objs.pop () : null_pointer_node,
> + target_targetsync.length () ? target_targetsync.pop () :
> null_pointer_node,
> + prefer_type.length () ? prefer_type.pop () : null_pointer_node, n_use,
> + use_objs.length () ? use_objs.pop () : null_pointer_node, n_destroy,
> + destroy_objs.length () ? destroy_objs.pop () : null_pointer_node, nowait,
> + depend);
And here too.
Add temporaries for the vars that need large expressions.
tree initarg = init_objs.length () ? init_objs.pop () : null_pointer_node;
tree syncarg = (target_targetsync.length ()
? target_targetsync.pop () : null_pointer_node);
tree preferarg
= prefer_type.length () ? prefer_type.pop () : null_pointer_node;
tree usearg = use_objs.length () ? use_objs.pop () : null_pointer_node;
tree destroyarg
= destroy_objs.length () ? destroy_objs.pop () : null_pointer_node;
gcall *call = gimple_build_call (fn, 11, device_num, n_init, initarg,
syncarg, preferarg, n_use, usearg,
n_destroy, destroyarg, nowait, depend);
or so.
> +void
> +GOMP_interop (int device_num, int n_init, void *init,
> + const void *target_targetsync, const void *prefer_type,
> + int n_use, void *use, int n_destroy, void *destroy,
> + unsigned int nowait, void **depend)
> +{
> + struct interop_data_t args;
> + args.device_num = device_num;
> + args.n_init = n_init;
> + args.n_use = n_use;
> + args.n_destroy = n_destroy;
> + if (n_init == 1)
> + {
> + args.init.item = (struct interop_obj_t **) init;
> + args.target_targetsync.val = (uintptr_t) target_targetsync;
> + args.prefer_type.val = (const char *) prefer_type;
> + }
> + else
> + {
> + args.init.items = (struct interop_obj_t ***) init;
> + args.target_targetsync.vals = (const int *) target_targetsync;
> + args.prefer_type.vals = (const char **) prefer_type;
> + }
> + if (n_use == 1)
> + args.use.item = (struct interop_obj_t *) use;
> + else
> + args.use.items = (struct interop_obj_t **) use;
> + if (n_destroy == 1)
> + args.destroy.item = (struct interop_obj_t **) destroy;
> + else
> + args.destroy.items = (struct interop_obj_t ***) destroy;
Tobias mentioned the n == 1 special-casing, but I thought it was about a single
conditional on the runtime side, now I see at least 8 such runtime tests,
some of them in loops.
Isn't that too much? Might be much simpler not to special case it,
or if it is really significant (I doubt, this is going to be expensive
call no matter what), make it dependent on just one test (either have the
simple form that allows <= 1 of everything, or one which allows arbitrary
numbers).
But I'd lean towards handling == 1 like everything else even if it is
the common case because then you don't need to test it everywhere.
Jakub
