Message ID | 20240527050626.3769230-2-tejas.belagod@arm.com |
---|---|
State | New |
Headers | show |
Series | AArch64/OpenMP: Test SVE ACLE types with various OpenMP constructs. | expand |
Tejas Belagod <tejas.belagod@arm.com> writes: > Currently poly-int type structures are passed by value to OpenMP runtime > functions for shared clauses etc. This patch improves on this by passing > around poly-int structures by address to avoid copy-overhead. > > gcc/ChangeLog > * omp-low.c (use_pointer_for_field): Use pointer if the OMP data > structure's field type is a poly-int. > --- > gcc/omp-low.cc | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc > index 1a65229cc37..b15607f4ef5 100644 > --- a/gcc/omp-low.cc > +++ b/gcc/omp-low.cc > @@ -466,7 +466,8 @@ static bool > use_pointer_for_field (tree decl, omp_context *shared_ctx) > { > if (AGGREGATE_TYPE_P (TREE_TYPE (decl)) > - || TYPE_ATOMIC (TREE_TYPE (decl))) > + || TYPE_ATOMIC (TREE_TYPE (decl)) > + || POLY_INT_CST_P (DECL_SIZE (decl))) > return true; > > /* We can only use copy-in/copy-out semantics for shared variables Realise this is also true of my original patch, but: I suppose a question here is whether this function is only ever used for local interfaces between code generated by the same source code function, or whether it's ABI in a more general sense. If the latter, I suppose we should make sure to handle ACLE types the same way regardless of whether the SVE vector size is known. (At the moment, the vector size is fixed for a TU, not just a function, but we should probably plan for relaxing that in future.) Thanks, Richard
On 5/30/24 6:28 PM, Richard Sandiford wrote: > Tejas Belagod <tejas.belagod@arm.com> writes: >> Currently poly-int type structures are passed by value to OpenMP runtime >> functions for shared clauses etc. This patch improves on this by passing >> around poly-int structures by address to avoid copy-overhead. >> >> gcc/ChangeLog >> * omp-low.c (use_pointer_for_field): Use pointer if the OMP data >> structure's field type is a poly-int. >> --- >> gcc/omp-low.cc | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc >> index 1a65229cc37..b15607f4ef5 100644 >> --- a/gcc/omp-low.cc >> +++ b/gcc/omp-low.cc >> @@ -466,7 +466,8 @@ static bool >> use_pointer_for_field (tree decl, omp_context *shared_ctx) >> { >> if (AGGREGATE_TYPE_P (TREE_TYPE (decl)) >> - || TYPE_ATOMIC (TREE_TYPE (decl))) >> + || TYPE_ATOMIC (TREE_TYPE (decl)) >> + || POLY_INT_CST_P (DECL_SIZE (decl))) >> return true; >> >> /* We can only use copy-in/copy-out semantics for shared variables > Thanks for the reviews. > Realise this is also true of my original patch, but: > > I suppose a question here is whether this function is only ever used for > local interfaces between code generated by the same source code function, > or whether it's ABI in a more general sense. I'm not a 100% sure, but AFAICS, 'use_pointer_for_field' seems to be used only for local interface between source and generated functions. I don't see any backend hooks into this or backend hooking into this function for general ABI. Ofcourse, I'm not the expert on OMP lowering, so it would be great to get an expert opinion on this. > If the latter, I suppose > we should make sure to handle ACLE types the same way regardless of > whether the SVE vector size is known. > When you say same way, do you mean the way SVE ABI defines the rules for SVE types? Thanks, Tejas. > (At the moment, the vector size is fixed for a TU, not just a function, > but we should probably plan for relaxing that in future.) > > Thanks, > Richard
Tejas Belagod <tejas.belagod@arm.com> writes: > On 5/30/24 6:28 PM, Richard Sandiford wrote: >> Tejas Belagod <tejas.belagod@arm.com> writes: >>> Currently poly-int type structures are passed by value to OpenMP runtime >>> functions for shared clauses etc. This patch improves on this by passing >>> around poly-int structures by address to avoid copy-overhead. >>> >>> gcc/ChangeLog >>> * omp-low.c (use_pointer_for_field): Use pointer if the OMP data >>> structure's field type is a poly-int. >>> --- >>> gcc/omp-low.cc | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc >>> index 1a65229cc37..b15607f4ef5 100644 >>> --- a/gcc/omp-low.cc >>> +++ b/gcc/omp-low.cc >>> @@ -466,7 +466,8 @@ static bool >>> use_pointer_for_field (tree decl, omp_context *shared_ctx) >>> { >>> if (AGGREGATE_TYPE_P (TREE_TYPE (decl)) >>> - || TYPE_ATOMIC (TREE_TYPE (decl))) >>> + || TYPE_ATOMIC (TREE_TYPE (decl)) >>> + || POLY_INT_CST_P (DECL_SIZE (decl))) >>> return true; >>> >>> /* We can only use copy-in/copy-out semantics for shared variables >> > > Thanks for the reviews. > >> Realise this is also true of my original patch, but: >> >> I suppose a question here is whether this function is only ever used for >> local interfaces between code generated by the same source code function, >> or whether it's ABI in a more general sense. > > I'm not a 100% sure, but AFAICS, 'use_pointer_for_field' seems to be > used only for local interface between source and generated functions. I > don't see any backend hooks into this or backend hooking into this > function for general ABI. Ofcourse, I'm not the expert on OMP lowering, > so it would be great to get an expert opinion on this. > >> If the latter, I suppose >> we should make sure to handle ACLE types the same way regardless of >> whether the SVE vector size is known. >> > > When you say same way, do you mean the way SVE ABI defines the rules for > SVE types? No, sorry, I meant that if the choice isn't purely local to a source code function, the condition should be something like sizeless_type_p (suitably abstracted) rather than POLY_INT_CST_P. That way, the "ABI" stays the same regardless of -msve-vector-bits. Thanks, Richard
On Fri, May 31, 2024 at 08:45:54AM +0100, Richard Sandiford wrote: > > When you say same way, do you mean the way SVE ABI defines the rules for > > SVE types? > > No, sorry, I meant that if the choice isn't purely local to a source > code function, the condition should be something like sizeless_type_p > (suitably abstracted) rather than POLY_INT_CST_P. That way, the "ABI" > stays the same regardless of -msve-vector-bits. There is no ABI, it is how the caller and indirect callee communicate, but both parts are compiled with the same compiler, so it can choose differently based on different compiler version etc. It is effectively simplified: struct whatever { ... }; void callee (void *x) { struct whatever *w = *x; use *w; } void caller (void) { struct whatever w; fill in w; ABI_call (callee, &w); } (plus in some cases the callee can also update values and propagate that back to caller). In any case, it is a similar "ABI" to e.g. tree-nested.cc communication between caller and nested callee, how exactly are the variables laid out in a struct depends on compiler version and whatever it decides, same compiler then emits both sides. Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Fri, May 31, 2024 at 08:45:54AM +0100, Richard Sandiford wrote: >> > When you say same way, do you mean the way SVE ABI defines the rules for >> > SVE types? >> >> No, sorry, I meant that if the choice isn't purely local to a source >> code function, the condition should be something like sizeless_type_p >> (suitably abstracted) rather than POLY_INT_CST_P. That way, the "ABI" >> stays the same regardless of -msve-vector-bits. > > There is no ABI, it is how the caller and indirect callee communicate, > but both parts are compiled with the same compiler, so it can choose > differently based on different compiler version etc. > It is effectively simplified: > struct whatever { ... }; > void callee (void *x) { struct whatever *w = *x; use *w; } > void caller (void) { struct whatever w; fill in w; ABI_call (callee, &w); } > (plus in some cases the callee can also update values and propagate that > back to caller). > In any case, it is a similar "ABI" to e.g. tree-nested.cc communication > between caller and nested callee, how exactly are the variables laid out > in a struct depends on compiler version and whatever it decides, same > compiler then emits both sides. Ah, ok, thanks. In that case I guess POLY_INT_CST_P should be safe/correct after all. Richard
On Mon, May 27, 2024 at 10:36:16AM +0530, Tejas Belagod wrote: > Currently poly-int type structures are passed by value to OpenMP runtime > functions for shared clauses etc. This patch improves on this by passing > around poly-int structures by address to avoid copy-overhead. > > gcc/ChangeLog > * omp-low.c (use_pointer_for_field): Use pointer if the OMP data > structure's field type is a poly-int. LGTM. > diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc > index 1a65229cc37..b15607f4ef5 100644 > --- a/gcc/omp-low.cc > +++ b/gcc/omp-low.cc > @@ -466,7 +466,8 @@ static bool > use_pointer_for_field (tree decl, omp_context *shared_ctx) > { > if (AGGREGATE_TYPE_P (TREE_TYPE (decl)) > - || TYPE_ATOMIC (TREE_TYPE (decl))) > + || TYPE_ATOMIC (TREE_TYPE (decl)) > + || POLY_INT_CST_P (DECL_SIZE (decl))) > return true; > > /* We can only use copy-in/copy-out semantics for shared variables > -- > 2.25.1 Jakub
diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc index 1a65229cc37..b15607f4ef5 100644 --- a/gcc/omp-low.cc +++ b/gcc/omp-low.cc @@ -466,7 +466,8 @@ static bool use_pointer_for_field (tree decl, omp_context *shared_ctx) { if (AGGREGATE_TYPE_P (TREE_TYPE (decl)) - || TYPE_ATOMIC (TREE_TYPE (decl))) + || TYPE_ATOMIC (TREE_TYPE (decl)) + || POLY_INT_CST_P (DECL_SIZE (decl))) return true; /* We can only use copy-in/copy-out semantics for shared variables