Only apply adjust_args in OpenMP dispatch if variant substitution occurs

Message ID 20250106161211.244121-1-parras@baylibre.com
State New
Headers
Series Only apply adjust_args in OpenMP dispatch if variant substitution occurs |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply

Commit Message

Paul-Antoine Arras Jan. 6, 2025, 4:12 p.m. UTC
  This is a followup to
084ea8ad584 OpenMP: middle-end support for dispatch + adjust_args.

This patch fixes a bug that caused arguments in an OpenMP dispatch call to be
modified even when no variant substitution occurred.

gcc/ChangeLog:

	* gimplify.cc (gimplify_call_expr): Create variable
	variant_substituted_p to control whether adjust_args applies.
---
 gcc/gimplify.cc | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
  

Comments

Paul-Antoine Arras Jan. 6, 2025, 4:20 p.m. UTC | #1
Apologies, I forgot to add the testcase. Please find attached an updated 
patch.

On 06/01/2025 17:12, Paul-Antoine Arras wrote:
> This is a followup to
> 084ea8ad584 OpenMP: middle-end support for dispatch + adjust_args.
> 
> This patch fixes a bug that caused arguments in an OpenMP dispatch call to be
> modified even when no variant substitution occurred.
> 
> gcc/ChangeLog:
> 
> 	* gimplify.cc (gimplify_call_expr): Create variable
> 	variant_substituted_p to control whether adjust_args applies.
> ---
>   gcc/gimplify.cc | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index bd324be926a..251d581f44c 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -3857,7 +3857,8 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value)
>     enum gimplify_status ret;
>     int i, nargs;
>     gcall *call;
> -  bool builtin_va_start_p = false, omp_dispatch_p = false;
> +  bool builtin_va_start_p = false, omp_dispatch_p = false,
> +       variant_substituted_p = false;
>     location_t loc = EXPR_LOCATION (*expr_p);
>   
>     gcc_assert (TREE_CODE (*expr_p) == CALL_EXPR);
> @@ -4035,7 +4036,10 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value)
>       {
>         tree variant = omp_resolve_declare_variant (fndecl);
>         if (variant != fndecl)
> -	CALL_EXPR_FN (*expr_p) = build1 (ADDR_EXPR, fnptrtype, variant);
> +	{
> +	  CALL_EXPR_FN (*expr_p) = build1 (ADDR_EXPR, fnptrtype, variant);
> +	  variant_substituted_p = true;
> +	}
>       }
>   
>     /* There is a sequence point before the call, so any side effects in
> @@ -4325,8 +4329,9 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value)
>   				}
>   			    }
>   
> -			  if ((need_device_ptr && !is_device_ptr)
> -			      || (need_device_addr && !has_device_addr))
> +			  if (variant_substituted_p
> +			      && ((need_device_ptr && !is_device_ptr)
> +				  || (need_device_addr && !has_device_addr)))
>   			    {
>   			      if (dispatch_device_num == NULL_TREE)
>   				{
  
Tobias Burnus Jan. 7, 2025, 10:16 a.m. UTC | #2
Hi PA,

Paul-Antoine Arras wrote:
> On 06/01/2025 17:12, Paul-Antoine Arras wrote:
>> This is a followup to
>> 084ea8ad584 OpenMP: middle-end support for dispatch + adjust_args.
>>
>> This patch fixes a bug that caused arguments in an OpenMP dispatch 
>> call to be
>> modified even when no variant substitution occurred.
>>
LGTM. Thanks!

Tobias
  

Patch

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index bd324be926a..251d581f44c 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -3857,7 +3857,8 @@  gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value)
   enum gimplify_status ret;
   int i, nargs;
   gcall *call;
-  bool builtin_va_start_p = false, omp_dispatch_p = false;
+  bool builtin_va_start_p = false, omp_dispatch_p = false,
+       variant_substituted_p = false;
   location_t loc = EXPR_LOCATION (*expr_p);
 
   gcc_assert (TREE_CODE (*expr_p) == CALL_EXPR);
@@ -4035,7 +4036,10 @@  gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value)
     {
       tree variant = omp_resolve_declare_variant (fndecl);
       if (variant != fndecl)
-	CALL_EXPR_FN (*expr_p) = build1 (ADDR_EXPR, fnptrtype, variant);
+	{
+	  CALL_EXPR_FN (*expr_p) = build1 (ADDR_EXPR, fnptrtype, variant);
+	  variant_substituted_p = true;
+	}
     }
 
   /* There is a sequence point before the call, so any side effects in
@@ -4325,8 +4329,9 @@  gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value)
 				}
 			    }
 
-			  if ((need_device_ptr && !is_device_ptr)
-			      || (need_device_addr && !has_device_addr))
+			  if (variant_substituted_p
+			      && ((need_device_ptr && !is_device_ptr)
+				  || (need_device_addr && !has_device_addr)))
 			    {
 			      if (dispatch_device_num == NULL_TREE)
 				{