Re: [PATCH] middle-end, i386: Pattern recognize add/subtract with carry [PR79173]

[Richard Biener <rguenther@suse.de>]

On Wed, 14 Jun 2023, Jakub Jelinek wrote:

> On Tue, Jun 13, 2023 at 01:29:04PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > > > +	  else if (addc_subc)
> > > > +	    {
> > > > +	      if (!integer_zerop (arg2))
> > > > +		;
> > > > +	      /* x = y + 0 + 0; x = y - 0 - 0; */
> > > > +	      else if (integer_zerop (arg1))
> > > > +		result = arg0;
> > > > +	      /* x = 0 + y + 0; */
> > > > +	      else if (subcode != MINUS_EXPR && integer_zerop (arg0))
> > > > +		result = arg1;
> > > > +	      /* x = y - y - 0; */
> > > > +	      else if (subcode == MINUS_EXPR
> > > > +		       && operand_equal_p (arg0, arg1, 0))
> > > > +		result = integer_zero_node;
> > > > +	    }
> > > 
> > > So this all performs simplifications but also constant folding.  In
> > > particular the match.pd re-simplification will invoke fold_const_call
> > > on all-constant argument function calls but does not do extra folding
> > > on partially constant arg cases but instead relies on patterns here.
> > > 
> > > Can you add all-constant arg handling to fold_const_call and
> > > consider moving cases like y + 0 + 0 to match.pd?
> > 
> > The reason I've done this here is that this is the spot where all other
> > similar internal functions are handled, be it the ubsan ones
> > - IFN_UBSAN_CHECK_{ADD,SUB,MUL}, or __builtin_*_overflow ones
> > - IFN_{ADD,SUB,MUL}_OVERFLOW, or these 2 new ones.  The code handles
> > there 2 constant arguments as well as various patterns that can be
> > simplified and has code to clean it up later, build a COMPLEX_CST,
> > or COMPLEX_EXPR etc. as needed.  So, I think we want to handle those
> > elsewhere, we should do it for all of those functions, but then
> > probably incrementally.
> 
> The patch I've posted yesterday now fully tested on x86_64-linux and
> i686-linux.
> 
> Here is an untested incremental patch to handle constant folding of these
> in fold-const-call.cc rather than gimple-fold.cc.
> Not really sure if that is the way to go because it is replacing 28
> lines of former code with 65 of new code, for the overall benefit that say
> int
> foo (long long *p)
> {
>   int one = 1;
>   long long max = __LONG_LONG_MAX__;
>   return __builtin_add_overflow (one, max, p);
> }
> can be now fully folded already in ccp1 pass while before it was only
> cleaned up in forwprop1 pass right after it.

I think that's still very much desirable so this followup looks OK.
Maybe you can re-base it as prerequesite though?

> As for doing some stuff in match.pd, I'm afraid it would result in even more
> significant growth, the advantage of gimple-fold.cc doing all of these in
> one place is that the needed infrastructure can be shared.

Yes, I saw that.

Richard.

> 
> --- gcc/gimple-fold.cc.jj	2023-06-14 12:21:38.657657759 +0200
> +++ gcc/gimple-fold.cc	2023-06-14 12:52:04.335054958 +0200
> @@ -5731,34 +5731,6 @@ gimple_fold_call (gimple_stmt_iterator *
>  	    result = arg0;
>  	  else if (subcode == MULT_EXPR && integer_onep (arg0))
>  	    result = arg1;
> -	  if (type
> -	      && result == NULL_TREE
> -	      && TREE_CODE (arg0) == INTEGER_CST
> -	      && TREE_CODE (arg1) == INTEGER_CST
> -	      && (!uaddc_usubc || TREE_CODE (arg2) == INTEGER_CST))
> -	    {
> -	      if (cplx_result)
> -		result = int_const_binop (subcode, fold_convert (type, arg0),
> -					  fold_convert (type, arg1));
> -	      else
> -		result = int_const_binop (subcode, arg0, arg1);
> -	      if (result && arith_overflowed_p (subcode, type, arg0, arg1))
> -		{
> -		  if (cplx_result)
> -		    overflow = build_one_cst (type);
> -		  else
> -		    result = NULL_TREE;
> -		}
> -	      if (uaddc_usubc && result)
> -		{
> -		  tree r = int_const_binop (subcode, result,
> -					    fold_convert (type, arg2));
> -		  if (r == NULL_TREE)
> -		    result = NULL_TREE;
> -		  else if (arith_overflowed_p (subcode, type, result, arg2))
> -		    overflow = build_one_cst (type);
> -		}
> -	    }
>  	  if (result)
>  	    {
>  	      if (result == integer_zero_node)
> --- gcc/fold-const-call.cc.jj	2023-06-02 10:36:43.096967505 +0200
> +++ gcc/fold-const-call.cc	2023-06-14 12:56:08.195631214 +0200
> @@ -1669,6 +1669,7 @@ fold_const_call (combined_fn fn, tree ty
>  {
>    const char *p0, *p1;
>    char c;
> +  tree_code subcode;
>    switch (fn)
>      {
>      case CFN_BUILT_IN_STRSPN:
> @@ -1738,6 +1739,46 @@ fold_const_call (combined_fn fn, tree ty
>      case CFN_FOLD_LEFT_PLUS:
>        return fold_const_fold_left (type, arg0, arg1, PLUS_EXPR);
>  
> +    case CFN_UBSAN_CHECK_ADD:
> +    case CFN_ADD_OVERFLOW:
> +      subcode = PLUS_EXPR;
> +      goto arith_overflow;
> +
> +    case CFN_UBSAN_CHECK_SUB:
> +    case CFN_SUB_OVERFLOW:
> +      subcode = MINUS_EXPR;
> +      goto arith_overflow;
> +
> +    case CFN_UBSAN_CHECK_MUL:
> +    case CFN_MUL_OVERFLOW:
> +      subcode = MULT_EXPR;
> +      goto arith_overflow;
> +
> +    arith_overflow:
> +      if (integer_cst_p (arg0) && integer_cst_p (arg1))
> +	{
> +	  tree itype
> +	    = TREE_CODE (type) == COMPLEX_TYPE ? TREE_TYPE (type) : type;
> +	  bool ovf = false;
> +	  tree r = int_const_binop (subcode, fold_convert (itype, arg0),
> +				    fold_convert (itype, arg1));
> +	  if (!r || TREE_CODE (r) != INTEGER_CST)
> +	    return NULL_TREE;
> +	  if (arith_overflowed_p (subcode, itype, arg0, arg1))
> +	    ovf = true;
> +	  if (TREE_OVERFLOW (r))
> +	    r = drop_tree_overflow (r);
> +	  if (itype == type)
> +	    {
> +	      if (ovf)
> +		return NULL_TREE;
> +	      return r;
> +	    }
> +	  else
> +	    return build_complex (type, r, build_int_cst (itype, ovf));
> +	}
> +      return NULL_TREE;
> +
>      default:
>        return fold_const_call_1 (fn, type, arg0, arg1);
>      }
> @@ -1896,6 +1937,30 @@ fold_const_call (combined_fn fn, tree ty
>  	return NULL_TREE;
>        }
>  
> +    case CFN_UADDC:
> +    case CFN_USUBC:
> +      if (integer_cst_p (arg0) && integer_cst_p (arg1) && integer_cst_p (arg2))
> +	{
> +	  tree itype = TREE_TYPE (type);
> +	  bool ovf = false;
> +	  tree_code subcode = fn == CFN_UADDC ? PLUS_EXPR : MINUS_EXPR;
> +	  tree r = int_const_binop (subcode, fold_convert (itype, arg0),
> +				    fold_convert (itype, arg1));
> +	  if (!r)
> +	    return NULL_TREE;
> +	  if (arith_overflowed_p (subcode, itype, arg0, arg1))
> +	    ovf = true;
> +	  tree r2 = int_const_binop (subcode, r, fold_convert (itype, arg2));
> +	  if (!r2 || TREE_CODE (r2) != INTEGER_CST)
> +	    return NULL_TREE;
> +	  if (arith_overflowed_p (subcode, itype, r, arg2))
> +	    ovf = true;
> +	  if (TREE_OVERFLOW (r2))
> +	    r2 = drop_tree_overflow (r2);
> +	  return build_complex (type, r2, build_int_cst (itype, ovf));
> +	}
> +      return NULL_TREE;
> +
>      default:
>        return fold_const_call_1 (fn, type, arg0, arg1, arg2);
>      }
> 
> 
> 	Jakub
> 
> 

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