[PATCH] Fix OMP CAS expansion with separate condition

[PATCH] Fix OMP CAS expansion with separate condition

[Richard Biener]

When forcing the condition to be split out from COND_EXPRs I see
a runtime failure of libgomp.fortran/atomic-19.f90 which can be
reduced to

  !$omp atomic update, compare, capture
  if (x == 69_2 - r) x = 6_8
    v = x

being miscompiled, the difference being

-  _13 = .ATOMIC_COMPARE_EXCHANGE (_9, _10, _11, 4, 0, 0);
-  _14 = IMAGPART_EXPR <_13>;
-  _15 = REALPART_EXPR <_13>;
-  _16 = _14 != 0 ? _11 : _15;
-  _2 = (integer(kind=4)) _16;
-  v_17 = _2;
+  _14 = .ATOMIC_COMPARE_EXCHANGE (_10, _11, _12, 4, 0, 0);
+  _15 = IMAGPART_EXPR <_14>;
+  _16 = REALPART_EXPR <_14>;
+  _2 = (logical(kind=1)) _15;
+  _3 = (integer(kind=4)) _16;
+  v_17 = _3;

where one can see a missing COND_EXPR.  It seems to be a latent
issue to me given the code can be exercised, it just maybe misses
a 'need_new' testcase combined with 'cond_stmt'.  Appearantly
the if (cond_stmt) code is just to avoid creating a temporary
(and possibly to preserve the condition compute if used elsewhere
since the original stmt is going to be deleted).  The following
makes the failure go away for me in my patched tree and it
also survives libgomp and gomp testing in an unpatched tree.

Full bootstrap & regtest running on x86_64-unknown-linux-gnu.

OK?

Thanks,
Richard.

2022-05-13  Richard Biener  <rguenther@suse.de>

	* omp-expand.cc (expand_omp_atomic_cas): Do not short-cut
	computation of the new value.
---
 gcc/omp-expand.cc | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gcc/omp-expand.cc b/gcc/omp-expand.cc
index ee708314793..9fcc67a3448 100644
--- a/gcc/omp-expand.cc
+++ b/gcc/omp-expand.cc
@@ -9092,16 +9092,17 @@ expand_omp_atomic_cas (basic_block load_bb, tree addr,
 
       if (cond_stmt)
 	{
-	  g = gimple_build_assign (gimple_assign_lhs (cond_stmt),
-				   NOP_EXPR, im);
+	  g = gimple_build_assign (cond, NOP_EXPR, im);
 	  gimple_set_location (g, loc);
 	  gsi_insert_before (&gsi, g, GSI_SAME_STMT);
 	}
-      else if (need_new)
+
+      if (need_new)
 	{
 	  g = gimple_build_assign (create_tmp_reg (itype), COND_EXPR,
-				   build2 (NE_EXPR, boolean_type_node,
-					   im, build_zero_cst (itype)),
+				   cond_stmt
+				   ? cond : build2 (NE_EXPR, boolean_type_node,
+						    im, build_zero_cst (itype)),
 				   d, re);
 	  gimple_set_location (g, loc);
 	  gsi_insert_before (&gsi, g, GSI_SAME_STMT);
-- 
2.35.3

Re: [PATCH] Fix OMP CAS expansion with separate condition

[Jakub Jelinek]

On Fri, May 13, 2022 at 12:23:01PM +0200, Richard Biener wrote:
> 2022-05-13  Richard Biener  <rguenther@suse.de>
> 
> 	* omp-expand.cc (expand_omp_atomic_cas): Do not short-cut
> 	computation of the new value.

Ok, thanks.

Though, depending on what exactly you allow or disallow, maybe even
the im != 0 might not be acceptable.
Oh, and if COND_EXPRs can only use some limited set of comparisons, we might
need to adjust e.g. arith_overflow_check_p and various other spots in
tree-ssa-math-opts.cc and other passes.

> diff --git a/gcc/omp-expand.cc b/gcc/omp-expand.cc
> index ee708314793..9fcc67a3448 100644
> --- a/gcc/omp-expand.cc
> +++ b/gcc/omp-expand.cc
> @@ -9092,16 +9092,17 @@ expand_omp_atomic_cas (basic_block load_bb, tree addr,
>  
>        if (cond_stmt)
>  	{
> -	  g = gimple_build_assign (gimple_assign_lhs (cond_stmt),
> -				   NOP_EXPR, im);
> +	  g = gimple_build_assign (cond, NOP_EXPR, im);
>  	  gimple_set_location (g, loc);
>  	  gsi_insert_before (&gsi, g, GSI_SAME_STMT);
>  	}
> -      else if (need_new)
> +
> +      if (need_new)
>  	{
>  	  g = gimple_build_assign (create_tmp_reg (itype), COND_EXPR,
> -				   build2 (NE_EXPR, boolean_type_node,
> -					   im, build_zero_cst (itype)),
> +				   cond_stmt
> +				   ? cond : build2 (NE_EXPR, boolean_type_node,
> +						    im, build_zero_cst (itype)),
>  				   d, re);
>  	  gimple_set_location (g, loc);
>  	  gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> -- 
> 2.35.3

	Jakub

Re: [PATCH] Fix OMP CAS expansion with separate condition

[Richard Biener]

On Thu, 19 May 2022, Jakub Jelinek wrote:

> On Fri, May 13, 2022 at 12:23:01PM +0200, Richard Biener wrote:
> > 2022-05-13  Richard Biener  <rguenther@suse.de>
> > 
> > 	* omp-expand.cc (expand_omp_atomic_cas): Do not short-cut
> > 	computation of the new value.
> 
> Ok, thanks.
> 
> Though, depending on what exactly you allow or disallow, maybe even
> the im != 0 might not be acceptable.
> Oh, and if COND_EXPRs can only use some limited set of comparisons, we might
> need to adjust e.g. arith_overflow_check_p and various other spots in
> tree-ssa-math-opts.cc and other passes.

With the changes I have in the queue the !cond_stmt path should be
effectively dead code (though I don't remove such immediately).

Not sure what you think the issue with arith_overflow_check_p would be?
Note this is only about COND_EXPRs in gimple assignments.

I'm going to push this OMP patch later but will delay pushing the
COND_EXPR patches until next week so I'm around more reliably to
deal with fallout.

Richard.

> > diff --git a/gcc/omp-expand.cc b/gcc/omp-expand.cc
> > index ee708314793..9fcc67a3448 100644
> > --- a/gcc/omp-expand.cc
> > +++ b/gcc/omp-expand.cc
> > @@ -9092,16 +9092,17 @@ expand_omp_atomic_cas (basic_block load_bb, tree addr,
> >  
> >        if (cond_stmt)
> >  	{
> > -	  g = gimple_build_assign (gimple_assign_lhs (cond_stmt),
> > -				   NOP_EXPR, im);
> > +	  g = gimple_build_assign (cond, NOP_EXPR, im);
> >  	  gimple_set_location (g, loc);
> >  	  gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> >  	}
> > -      else if (need_new)
> > +
> > +      if (need_new)
> >  	{
> >  	  g = gimple_build_assign (create_tmp_reg (itype), COND_EXPR,
> > -				   build2 (NE_EXPR, boolean_type_node,
> > -					   im, build_zero_cst (itype)),
> > +				   cond_stmt
> > +				   ? cond : build2 (NE_EXPR, boolean_type_node,
> > +						    im, build_zero_cst (itype)),
> >  				   d, re);
> >  	  gimple_set_location (g, loc);
> >  	  gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> > -- 
> > 2.35.3
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] Fix OMP CAS expansion with separate condition

[Jakub Jelinek]

On Thu, May 19, 2022 at 02:13:33PM +0000, Richard Biener wrote:
> > Though, depending on what exactly you allow or disallow, maybe even
> > the im != 0 might not be acceptable.
> > Oh, and if COND_EXPRs can only use some limited set of comparisons, we might
> > need to adjust e.g. arith_overflow_check_p and various other spots in
> > tree-ssa-math-opts.cc and other passes.
> 
> With the changes I have in the queue the !cond_stmt path should be
> effectively dead code (though I don't remove such immediately).
> 
> Not sure what you think the issue with arith_overflow_check_p would be?
> Note this is only about COND_EXPRs in gimple assignments.

Just that there will be more dead code in the compiler.
At least tree-ssa-math-opts.cc in various spots (but I think phiopt too and
other spots I've touched too) are looking for all 3 kinds of comparisons,
GIMPLE_CONDs, comparisons in COND_EXPR first operand and assignments
with tcc_comparison rhs codes and deal with all 3.  If we restrict what
can appear in COND_EXPR first operand in gimple assignments, perhaps we'll
need to only deal with the first and last case and not the COND_EXPR ones.

	Jakub

Re: [PATCH] Fix OMP CAS expansion with separate condition

[Richard Biener]

On Thu, 19 May 2022, Jakub Jelinek wrote:

> On Thu, May 19, 2022 at 02:13:33PM +0000, Richard Biener wrote:
> > > Though, depending on what exactly you allow or disallow, maybe even
> > > the im != 0 might not be acceptable.
> > > Oh, and if COND_EXPRs can only use some limited set of comparisons, we might
> > > need to adjust e.g. arith_overflow_check_p and various other spots in
> > > tree-ssa-math-opts.cc and other passes.
> > 
> > With the changes I have in the queue the !cond_stmt path should be
> > effectively dead code (though I don't remove such immediately).
> > 
> > Not sure what you think the issue with arith_overflow_check_p would be?
> > Note this is only about COND_EXPRs in gimple assignments.
> 
> Just that there will be more dead code in the compiler.
> At least tree-ssa-math-opts.cc in various spots (but I think phiopt too and
> other spots I've touched too) are looking for all 3 kinds of comparisons,
> GIMPLE_CONDs, comparisons in COND_EXPR first operand and assignments
> with tcc_comparison rhs codes and deal with all 3.  If we restrict what
> can appear in COND_EXPR first operand in gimple assignments, perhaps we'll
> need to only deal with the first and last case and not the COND_EXPR ones.

Ah, yeah - there will be followup cleanup opportunities like shaving
off 5% of gimple-match.cc when I'm happy with a required phiopt
adjustment.

Richard.