Re: [Patch] OpenMP: Support complex/float in && and || reduction

[Jakub Jelinek <jakub@redhat.com>]

On Sat, May 01, 2021 at 01:12:15AM +0200, Tobias Burnus wrote:
> gcc/c/ChangeLog:
> 
> 	* c-typeck.c (c_finish_omp_clauses): Accept float + complex for || and &&
> 	reductions.
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.c (finish_omp_reduction_clause): Accept float + complex for || and &&
> 	reductions.
> 
> gcc/ChangeLog:
> 
> 	* omp-low.c (lower_rec_input_clauses, lower_reduction_clauses): Handle && and ||
> 	with floating-point and complex arguments.

All the above ChangeLog lines are too long.

> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -6376,6 +6376,11 @@ lower_rec_input_clauses (tree clauses, gimple_seq *ilist, gimple_seq *dlist,
>  		  if (code == MINUS_EXPR)
>  		    code = PLUS_EXPR;
>  
> +		  /* C/C++ permits FP/complex with || and &&.  */
> +		  bool is_fp_and_or
> +		    = ((code == TRUTH_ANDIF_EXPR || code == TRUTH_ORIF_EXPR)
> +		       && (FLOAT_TYPE_P (TREE_TYPE (new_var))
> +			   || TREE_CODE (TREE_TYPE (new_var)) == COMPLEX_TYPE));

The above line is too long too, please use
			   || (TREE_CODE (TREE_TYPE (new_var))
			       == COMPLEX_TYPE)));

>  		  tree new_vard = new_var;
>  		  if (is_simd && omp_is_reference (var))
>  		    {
> @@ -6443,8 +6448,23 @@ lower_rec_input_clauses (tree clauses, gimple_seq *ilist, gimple_seq *dlist,
>  		      if (is_simd)
>  			{
>  			  tree ref = build_outer_var_ref (var, ctx);
> -
> -			  x = build2 (code, TREE_TYPE (ref), ref, new_var);
> +			  tree new_var2 = new_var;
> +			  if (is_fp_and_or)
> +			    new_var2 = fold_build2_loc (
> +					 clause_loc, NE_EXPR,
> +					 integer_type_node, new_var,
> +					 build_zero_cst (TREE_TYPE (new_var)));

Formatting, would be nice to avoid the ( at the end of line, e.g.
			    {
			      tree zero = build_zero_cst (TREE_TYPE (new_var));
			      new_var2
				= fold_build2_loc (clause_loc, NE_EXPR,
						   integer_type_node, new_var,
						   zero);
			    }

> +			  tree ref2 = ref;
> +			  if (is_fp_and_or
> +			      && (FLOAT_TYPE_P (TREE_TYPE (ref))
> +				  || TREE_CODE (TREE_TYPE (ref))
> +				     == COMPLEX_TYPE))

Please wrap the == into ()s.
Though ref should have the same type new_var (or at least a compatible type),
so I don't see the point of the && ... in there and of using two separate
if (is_fp_and_or) blocks.
So
tree new_var2 = new_var;
tree ref2 = ref;
if (is_fp_and_or)
  {
    tree zero = ...;
    new_var2 = ...
    ref2 = ...;
  }

> +			    ref2 = fold_build2_loc (
> +					 clause_loc, NE_EXPR, integer_type_node,
> +					 ref, build_zero_cst (TREE_TYPE (ref)));

And try to avoid the ( here too.
Even better would be to split the function a little bit, but that can be
done another day.

> +			  x = build2 (code, TREE_TYPE (ref2), ref2, new_var2);
> +			  if (new_var2 != new_var)
> +			    x = fold_convert (TREE_TYPE (new_var), x);
>  			  ref = build_outer_var_ref (var, ctx);
>  			  gimplify_assign (ref, x, dlist);
>  			}
> @@ -7384,13 +7404,32 @@ lower_reduction_clauses (tree clauses, gimple_seq *stmt_seqp,
>        if (code == MINUS_EXPR)
>          code = PLUS_EXPR;
>  
> +      /* C/C++ permits FP/complex with || and &&.  */
> +      bool is_fp_and_or = ((code == TRUTH_ANDIF_EXPR || code == TRUTH_ORIF_EXPR)
> +			   && (FLOAT_TYPE_P (TREE_TYPE (new_var))
> +			       || TREE_CODE (TREE_TYPE (new_var))
> +				   == COMPLEX_TYPE));

Again, ()s around ==.

>        if (count == 1)
>  	{
>  	  tree addr = build_fold_addr_expr_loc (clause_loc, ref);
>  
>  	  addr = save_expr (addr);
>  	  ref = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (addr)), addr);
> -	  x = fold_build2_loc (clause_loc, code, TREE_TYPE (ref), ref, new_var);
> +	  tree new_var2 = new_var;
> +	  if (is_fp_and_or)
> +	    new_var2 = fold_build2_loc (clause_loc, NE_EXPR,
> +					integer_type_node, new_var,
> +					build_zero_cst (TREE_TYPE (new_var)));
> +	  tree ref2 = ref;
> +	  if (is_fp_and_or
> +	      && (FLOAT_TYPE_P (TREE_TYPE (ref))
> +		  || TREE_CODE (TREE_TYPE (ref)) == COMPLEX_TYPE))
> +	    ref2 = fold_build2_loc (clause_loc, NE_EXPR, integer_type_node, ref,
> +				    build_zero_cst (TREE_TYPE (ref)));

And similar question as above.
And the line is too long.

> +	  x = fold_build2_loc (clause_loc, code, TREE_TYPE (new_var2), ref2,
> +			       new_var2);
> +	  if (new_var2 != new_var)
> +	    x = fold_convert (TREE_TYPE (new_var), x);
>  	  x = build2 (OMP_ATOMIC, void_type_node, addr, x);
>  	  OMP_ATOMIC_MEMORY_ORDER (x) = OMP_MEMORY_ORDER_RELAXED;
>  	  gimplify_and_add (x, stmt_seqp);
> @@ -7495,7 +7534,21 @@ lower_reduction_clauses (tree clauses, gimple_seq *stmt_seqp,
>  	    }
>  	  else
>  	    {
> -	      x = build2 (code, TREE_TYPE (out), out, priv);
> +	      tree out2 = out;
> +	      if (is_fp_and_or)
> +		out2 = fold_build2_loc (clause_loc, NE_EXPR,
> +					integer_type_node, out,
> +					build_zero_cst (type));
> +	      tree priv2 = priv;
> +	      if (is_fp_and_or
> +		  && (FLOAT_TYPE_P (TREE_TYPE (priv))
> +		      || TREE_CODE (TREE_TYPE (priv)) == COMPLEX_TYPE))

And here too.

> +		priv2 = fold_build2_loc (clause_loc, NE_EXPR,
> +					 integer_type_node, priv,
> +					 build_zero_cst (TREE_TYPE (priv)));
> +	      x = build2 (code, TREE_TYPE (out2), out2, priv2);
> +	      if (out2 != out)
> +		x = fold_convert (TREE_TYPE (out), x);
>  	      out = unshare_expr (out);
>  	      gimplify_assign (out, x, &sub_seq);
>  	    }
> @@ -7529,7 +7582,20 @@ lower_reduction_clauses (tree clauses, gimple_seq *stmt_seqp,
>  	}
>        else
>  	{
> -	  x = build2 (code, TREE_TYPE (ref), ref, new_var);
> +	  tree new_var2 = new_var;
> +	  if (is_fp_and_or)
> +	    new_var2 = fold_build2_loc (clause_loc, NE_EXPR,
> +					integer_type_node, new_var,
> +					build_zero_cst (TREE_TYPE (new_var)));
> +	  tree ref2 = ref;
> +	  if (is_fp_and_or
> +	      && (FLOAT_TYPE_P (TREE_TYPE (ref))
> +		  || TREE_CODE (TREE_TYPE (ref)) == COMPLEX_TYPE))
> +	    ref2 = fold_build2_loc (clause_loc, NE_EXPR, integer_type_node, ref,
> +					build_zero_cst (TREE_TYPE (ref)));
> +	  x = build2 (code, TREE_TYPE (ref), ref2, new_var2);
> +	  if (new_var2 != new_var)
> +	    x = fold_convert (TREE_TYPE (new_var), x);

Likewise.

For the testcases, would be nice to have one with _Complex int, though
perhaps separately from the ones you've included because while float
or _Complex double are standard, _Complex int is a GNU extension.

Otherwise LGTM.

	Jakub