RE: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values

[Richard Biener <rguenther@suse.de>]

On Mon, 25 Oct 2021, Tamar Christina wrote:

> > -----Original Message-----
> > From: Richard Biener <rguenther@suse.de>
> > Sent: Friday, October 15, 2021 12:31 PM
> > To: Tamar Christina <Tamar.Christina@arm.com>
> > Cc: gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; nd
> > <nd@arm.com>
> > Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns
> > on signed values
> > 
> > On Fri, 15 Oct 2021, Tamar Christina wrote:
> > 
> > > Hi All,
> > >
> > > During testing after rebasing to commit I noticed a failing testcase
> > > with the bitmask compare patch.
> > >
> > > Consider the following C++ testcase:
> > >
> > > #include <compare>
> > >
> > > #define A __attribute__((noipa))
> > > A bool f5 (double i, double j) { auto c = i <=> j; return c >= 0; }
> > >
> > > This turns into a comparison against chars, on systems where chars are
> > > signed the pattern inserts an unsigned convert such that it's able to
> > > do the transformation.
> > >
> > > i.e.:
> > >
> > >   # RANGE [-1, 2]
> > >   # c$_M_value_22 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > >   # RANGE ~[3, 254]
> > >   _11 = (unsigned char) c$_M_value_22;
> > >   _19 = _11 <= 1;
> > >   # .MEM_24 = VDEF <.MEM_6(D)>
> > >   D.10434 ={v} {CLOBBER};
> > >   # .MEM_14 = VDEF <.MEM_24>
> > >   D.10407 ={v} {CLOBBER};
> > >   # VUSE <.MEM_14>
> > >   return _19;
> > >
> > > instead of:
> > >
> > >   # RANGE [-1, 2]
> > >   # c$_M_value_5 = PHI <-1(3), 0(2), 2(5), 1(4)>
> > >   # RANGE [-2, 2]
> > >   _3 = c$_M_value_5 & -2;
> > >   _19 = _3 == 0;
> > >   # .MEM_24 = VDEF <.MEM_6(D)>
> > >   D.10440 ={v} {CLOBBER};
> > >   # .MEM_14 = VDEF <.MEM_24>
> > >   D.10413 ={v} {CLOBBER};
> > >   # VUSE <.MEM_14>
> > >   return _19;
> > >
> > > This causes much worse codegen under -ffast-math due to phiops no
> > > longer recognizing the pattern.  It turns out that phiopts
> > > spaceship_replacement is looking for the exact form that was just changed.
> > >
> > > Trying to get it to recognize the new form is not trivial as the
> > > transformation doesn't look to work when the thing it's pointing to is itself
> > a phi-node.
> > 
> > What do you mean?  Where it handles the BIT_AND it could also handle the
> > conversion, no?  The later handling would probably more explicitely need to
> > distinguish between the BIT_AND and the conversion forms.
> 
> Looks like I misunderstood the code, it was looking at the uses not the defs of
> the value.
> 
> --- inline copy of patch ---
> 
> The comments seems to suggest this code only checks for (res & ~1) == 0 but the
> implementation seems to suggest it's broader.
> 
> As such I added a case to check to see if the value comparison we found is a
> type cast.  and strips away the type cast and continues.
> 
> In match.pd the typecasts are only added for signed comparisons to == 0 and != 0
> which are then rewritten into comparisons with 1.
> 
> As such I only check for 1 and LE and GT, which is what match.pd would have
> rewritten it to.
> 
> This fixes the regression but this is not code I 100% understand, since I don't
> really know the semantics of the spaceship operator so would appreciate an extra
> look.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu,
> x86_64-pc-linux-gnu and no regressions.
> 
> Ok for master?

Please add a testcase.  I hope Jakub can review the spaceship_replacement
patch since he's the one familiar with the code.

Thanks,
Richard.

> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 	* tree-ssa-phiopt.c (spaceship_replacement): Handle new canonical
> 	codegen.
> 
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index 0e339c46afa29fa97f90d9bc4394370cd9b4b396..65b25be3399b75d5e9cab0f78aa2340418571a33 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -2037,6 +2037,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
>    tree lhs, rhs;
>    gimple *orig_use_stmt = use_stmt;
>    tree orig_use_lhs = NULL_TREE;
> +  bool is_canon = false;
>    int prec = TYPE_PRECISION (TREE_TYPE (phires));
>    if (is_gimple_assign (use_stmt)
>        && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR
> @@ -2063,6 +2064,26 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
>      }
>    else if (is_gimple_assign (use_stmt))
>      {
> +      /* Deal with if match.pd has rewritten the (res & ~1) == 0
> +	 into res <= 1 and has left a type-cast for signed types.  */
> +      if (gimple_assign_cast_p (use_stmt))
> +	{
> +	  orig_use_lhs = gimple_assign_lhs (use_stmt);
> +	  if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs))
> +	    return false;
> +	  if (EDGE_COUNT (phi_bb->preds) != 4)
> +	    return false;
> +	  if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)))
> +	    return false;
> +	  if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt))
> +	    return false;
> +	  tree_code cmp;
> +	  if (is_gimple_assign (use_stmt)
> +	      && (cmp = gimple_assign_rhs_code (use_stmt))
> +	      && (cmp == LE_EXPR || cmp == GT_EXPR)
> +	      && wi::eq_p (wi::to_wide (gimple_assign_rhs2 (use_stmt)), 1))
> +	    is_canon = true;
> +	}
>        if (gimple_assign_rhs_class (use_stmt) == GIMPLE_BINARY_RHS)
>  	{
>  	  cmp = gimple_assign_rhs_code (use_stmt);
> @@ -2099,7 +2120,9 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
>        || !tree_fits_shwi_p (rhs)
>        || !IN_RANGE (tree_to_shwi (rhs), -1, 1))
>      return false;
> -  if (orig_use_lhs)
> +  /* If we're already in the canonical form we need to keep the original
> +     comparison.  */
> +  if (orig_use_lhs && !is_canon)
>      {
>        if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs))
>  	return false;
> @@ -2310,6 +2333,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
>      one_cmp = GT_EXPR;
>  
>    enum tree_code res_cmp;
> +
>    switch (cmp)
>      {
>      case EQ_EXPR:
> @@ -2345,6 +2369,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
>  	res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR;
>        else if (integer_minus_onep (rhs))
>  	res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR;
> +      else if (integer_onep (rhs) && is_canon)
> +	res_cmp = GE_EXPR;
>        else
>  	return false;
>        break;
> @@ -2353,6 +2379,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
>  	res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR;
>        else if (integer_zerop (rhs))
>  	res_cmp = one_cmp;
> +      else if (integer_onep (rhs) && is_canon)
> +	res_cmp = LE_EXPR;
>        else
>  	return false;
>        break;
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)