diff mbox series

[01/11] OpenMP/PolyInt: Pass poly-int structures by address to OMP libs.

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

Commit Message

Tejas Belagod May 27, 2024, 5:06 a.m. UTC 
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(-)

Comments

Richard Sandiford May 30, 2024, 12:58 p.m. UTC | #1 
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
Tejas Belagod May 31, 2024, 6:30 a.m. UTC | #2 
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
Richard Sandiford May 31, 2024, 7:45 a.m. UTC | #3 
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
Jakub Jelinek May 31, 2024, 8:01 a.m. UTC | #4 
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
Richard Sandiford May 31, 2024, 8:23 a.m. UTC | #5 
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
diff mbox series

Patch

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