Ping: [PATCHv3] aarch64/expr: Use ccmp when the outer expression is used twice [PR100942]

[Richard Sandiford <richard.sandiford@arm.com>]

Ping for the expr/cfgexpand bits

Richard Sandiford <richard.sandiford@arm.com> writes:
> Andrew Pinski <quic_apinski@quicinc.com> writes:
>> Ccmp is not used if the result of the and/ior is used by both
>> a GIMPLE_COND and a GIMPLE_ASSIGN. This improves the code generation
>> here by using ccmp in this case.
>> Two changes is required, first we need to allow the outer statement's
>> result be used more than once.
>> The second change is that during the expansion of the gimple, we need
>> to try using ccmp. This is needed because we don't use expand the ssa
>> name of the lhs but rather expand directly from the gimple.
>>
>> A small note on the ccmp_4.c testcase, we should be able to get slightly
>> better than with this patch but it is one extra instruction compared to
>> before.
>>
>> Diff from v1:
>> * v2: Split out expand_gimple_assign_ssa so the we only need to handle
>> promotion once. Add ccmp_5.c testcase which was suggested. Change comment
>> on ccmp_candidate_p.
>
> I meant more that we should split out the gassign handling in
> expand_expr_real_1, since we're effectively making cfgexpand follow
> it more closely.  What do you think about the attached version?
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.
>
> OK for the expr/cfgexpand bits?
>
> Thanks,
> Richard
>
> ----
>
> Ccmp is not used if the result of the and/ior is used by both
> a GIMPLE_COND and a GIMPLE_ASSIGN. This improves the code generation
> here by using ccmp in this case.
> Two changes is required, first we need to allow the outer statement's
> result be used more than once.
> The second change is that during the expansion of the gimple, we need
> to try using ccmp. This is needed because we don't use expand the ssa
> name of the lhs but rather expand directly from the gimple.
>
> A small note on the ccmp_4.c testcase, we should be able to get slightly
> better than with this patch but it is one extra instruction compared to
> before.
>
> 	PR target/100942
>
> gcc/ChangeLog:
>
> 	* ccmp.cc (ccmp_candidate_p): Add outer argument.
> 	Allow if the outer is true and the lhs is used more
> 	than once.
> 	(expand_ccmp_expr): Update call to ccmp_candidate_p.
> 	* expr.h (expand_expr_real_gassign): Declare.
> 	* expr.cc (expand_expr_real_gassign): New function, split out from...
> 	(expand_expr_real_1): ...here.
> 	* cfgexpand.cc (expand_gimple_stmt_1): Use expand_expr_real_gassign.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/ccmp_3.c: New test.
> 	* gcc.target/aarch64/ccmp_4.c: New test.
> 	* gcc.target/aarch64/ccmp_5.c: New test.
>
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> Co-authored-by: Richard Sandiford <richard.sandiford@arm.com>
> ---
>  gcc/ccmp.cc                               |  12 +--
>  gcc/cfgexpand.cc                          |  31 ++-----
>  gcc/expr.cc                               | 103 ++++++++++++----------
>  gcc/expr.h                                |   3 +
>  gcc/testsuite/gcc.target/aarch64/ccmp_3.c |  20 +++++
>  gcc/testsuite/gcc.target/aarch64/ccmp_4.c |  35 ++++++++
>  gcc/testsuite/gcc.target/aarch64/ccmp_5.c |  20 +++++
>  7 files changed, 149 insertions(+), 75 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_3.c
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_4.c
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_5.c
>
> diff --git a/gcc/ccmp.cc b/gcc/ccmp.cc
> index 09d6b5595a4..7cb525addf4 100644
> --- a/gcc/ccmp.cc
> +++ b/gcc/ccmp.cc
> @@ -90,9 +90,10 @@ ccmp_tree_comparison_p (tree t, basic_block bb)
>     If all checks OK in expand_ccmp_expr, it emits insns in prep_seq, then
>     insns in gen_seq.  */
>  
> -/* Check whether G is a potential conditional compare candidate.  */
> +/* Check whether G is a potential conditional compare candidate; OUTER is true if
> +   G is the outer most AND/IOR.  */
>  static bool
> -ccmp_candidate_p (gimple *g)
> +ccmp_candidate_p (gimple *g, bool outer = false)
>  {
>    tree lhs, op0, op1;
>    gimple *gs0, *gs1;
> @@ -109,8 +110,9 @@ ccmp_candidate_p (gimple *g)
>    lhs = gimple_assign_lhs (g);
>    op0 = gimple_assign_rhs1 (g);
>    op1 = gimple_assign_rhs2 (g);
> -  if ((TREE_CODE (op0) != SSA_NAME) || (TREE_CODE (op1) != SSA_NAME)
> -      || !has_single_use (lhs))
> +  if ((TREE_CODE (op0) != SSA_NAME) || (TREE_CODE (op1) != SSA_NAME))
> +    return false;
> +  if (!outer && !has_single_use (lhs))
>      return false;
>  
>    bb = gimple_bb (g);
> @@ -284,7 +286,7 @@ expand_ccmp_expr (gimple *g, machine_mode mode)
>    rtx_insn *last;
>    rtx tmp;
>  
> -  if (!ccmp_candidate_p (g))
> +  if (!ccmp_candidate_p (g, true))
>      return NULL_RTX;
>  
>    last = get_last_insn ();
> diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
> index 1db22f0a1a3..381ed2c82d7 100644
> --- a/gcc/cfgexpand.cc
> +++ b/gcc/cfgexpand.cc
> @@ -3971,37 +3971,18 @@ expand_gimple_stmt_1 (gimple *stmt)
>  	  {
>  	    rtx target, temp;
>  	    bool nontemporal = gimple_assign_nontemporal_move_p (assign_stmt);
> -	    struct separate_ops ops;
>  	    bool promoted = false;
>  
>  	    target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE);
>  	    if (GET_CODE (target) == SUBREG && SUBREG_PROMOTED_VAR_P (target))
>  	      promoted = true;
>  
> -	    ops.code = gimple_assign_rhs_code (assign_stmt);
> -	    ops.type = TREE_TYPE (lhs);
> -	    switch (get_gimple_rhs_class (ops.code))
> -	      {
> -		case GIMPLE_TERNARY_RHS:
> -		  ops.op2 = gimple_assign_rhs3 (assign_stmt);
> -		  /* Fallthru */
> -		case GIMPLE_BINARY_RHS:
> -		  ops.op1 = gimple_assign_rhs2 (assign_stmt);
> -		  /* Fallthru */
> -		case GIMPLE_UNARY_RHS:
> -		  ops.op0 = gimple_assign_rhs1 (assign_stmt);
> -		  break;
> -		default:
> -		  gcc_unreachable ();
> -	      }
> -	    ops.location = gimple_location (stmt);
> -
> -	    /* If we want to use a nontemporal store, force the value to
> -	       register first.  If we store into a promoted register,
> -	       don't directly expand to target.  */
> +	   /* If we want to use a nontemporal store, force the value to
> +	      register first.  If we store into a promoted register,
> +	      don't directly expand to target.  */
>  	    temp = nontemporal || promoted ? NULL_RTX : target;
> -	    temp = expand_expr_real_2 (&ops, temp, GET_MODE (target),
> -				       EXPAND_NORMAL);
> +	    temp = expand_expr_real_gassign (assign_stmt, temp,
> +					     GET_MODE (target), EXPAND_NORMAL);
>  
>  	    if (temp == target)
>  	      ;
> @@ -4013,7 +3994,7 @@ expand_gimple_stmt_1 (gimple *stmt)
>  		if (CONSTANT_P (temp) && GET_MODE (temp) == VOIDmode)
>  		  {
>  		    temp = convert_modes (GET_MODE (target),
> -					  TYPE_MODE (ops.type),
> +					  TYPE_MODE (TREE_TYPE (lhs)),
>  					  temp, unsignedp);
>  		    temp = convert_modes (GET_MODE (SUBREG_REG (target)),
>  					  GET_MODE (target), temp, unsignedp);
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index dc816bc20fa..f9052a6ff5f 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -11035,6 +11035,62 @@ stmt_is_replaceable_p (gimple *stmt)
>    return false;
>  }
>  
> +/* A subroutine of expand_expr_real_1.  Expand gimple assignment G,
> +   which is known to set an SSA_NAME result.  The other arguments are
> +   as for expand_expr_real_1.  */
> +
> +rtx
> +expand_expr_real_gassign (gassign *g, rtx target, machine_mode tmode,
> +			  enum expand_modifier modifier, rtx *alt_rtl,
> +			  bool inner_reference_p)
> +{
> +  separate_ops ops;
> +  rtx r;
> +  location_t saved_loc = curr_insn_location ();
> +  auto loc = gimple_location (g);
> +  if (loc != UNKNOWN_LOCATION)
> +    set_curr_insn_location (loc);
> +  tree lhs = gimple_assign_lhs (g);
> +  ops.code = gimple_assign_rhs_code (g);
> +  ops.type = TREE_TYPE (lhs);
> +  switch (get_gimple_rhs_class (ops.code))
> +    {
> +    case GIMPLE_TERNARY_RHS:
> +      ops.op2 = gimple_assign_rhs3 (g);
> +      /* Fallthru */
> +    case GIMPLE_BINARY_RHS:
> +      ops.op1 = gimple_assign_rhs2 (g);
> +
> +      /* Try to expand conditonal compare.  */
> +      if (targetm.gen_ccmp_first)
> +	{
> +	  gcc_checking_assert (targetm.gen_ccmp_next != NULL);
> +	  r = expand_ccmp_expr (g, TYPE_MODE (ops.type));
> +	  if (r)
> +	    break;
> +	}
> +      /* Fallthru */
> +    case GIMPLE_UNARY_RHS:
> +      ops.op0 = gimple_assign_rhs1 (g);
> +      ops.location = loc;
> +      r = expand_expr_real_2 (&ops, target, tmode, modifier);
> +      break;
> +    case GIMPLE_SINGLE_RHS:
> +      {
> +	r = expand_expr_real (gimple_assign_rhs1 (g), target,
> +			      tmode, modifier, alt_rtl,
> +			      inner_reference_p);
> +	break;
> +      }
> +    default:
> +      gcc_unreachable ();
> +    }
> +  set_curr_insn_location (saved_loc);
> +  if (REG_P (r) && !REG_EXPR (r))
> +    set_reg_attrs_for_decl_rtl (lhs, r);
> +  return r;
> +}
> +
>  rtx
>  expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
>  		    enum expand_modifier modifier, rtx *alt_rtl,
> @@ -11198,51 +11254,8 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
>  	  && stmt_is_replaceable_p (SSA_NAME_DEF_STMT (exp)))
>  	g = SSA_NAME_DEF_STMT (exp);
>        if (g)
> -	{
> -	  rtx r;
> -	  location_t saved_loc = curr_insn_location ();
> -	  loc = gimple_location (g);
> -	  if (loc != UNKNOWN_LOCATION)
> -	    set_curr_insn_location (loc);
> -	  ops.code = gimple_assign_rhs_code (g);
> -          switch (get_gimple_rhs_class (ops.code))
> -	    {
> -	    case GIMPLE_TERNARY_RHS:
> -	      ops.op2 = gimple_assign_rhs3 (g);
> -	      /* Fallthru */
> -	    case GIMPLE_BINARY_RHS:
> -	      ops.op1 = gimple_assign_rhs2 (g);
> -
> -	      /* Try to expand conditonal compare.  */
> -	      if (targetm.gen_ccmp_first)
> -		{
> -		  gcc_checking_assert (targetm.gen_ccmp_next != NULL);
> -		  r = expand_ccmp_expr (g, mode);
> -		  if (r)
> -		    break;
> -		}
> -	      /* Fallthru */
> -	    case GIMPLE_UNARY_RHS:
> -	      ops.op0 = gimple_assign_rhs1 (g);
> -	      ops.type = TREE_TYPE (gimple_assign_lhs (g));
> -	      ops.location = loc;
> -	      r = expand_expr_real_2 (&ops, target, tmode, modifier);
> -	      break;
> -	    case GIMPLE_SINGLE_RHS:
> -	      {
> -		r = expand_expr_real (gimple_assign_rhs1 (g), target,
> -				      tmode, modifier, alt_rtl,
> -				      inner_reference_p);
> -		break;
> -	      }
> -	    default:
> -	      gcc_unreachable ();
> -	    }
> -	  set_curr_insn_location (saved_loc);
> -	  if (REG_P (r) && !REG_EXPR (r))
> -	    set_reg_attrs_for_decl_rtl (SSA_NAME_VAR (exp), r);
> -	  return r;
> -	}
> +	return expand_expr_real_gassign (as_a<gassign *> (g), target, tmode,
> +					 modifier, alt_rtl, inner_reference_p);
>  
>        ssa_name = exp;
>        decl_rtl = get_rtx_for_ssa_name (ssa_name);
> diff --git a/gcc/expr.h b/gcc/expr.h
> index f04f40da6ab..64956f63029 100644
> --- a/gcc/expr.h
> +++ b/gcc/expr.h
> @@ -302,6 +302,9 @@ extern rtx expand_expr_real_1 (tree, rtx, machine_mode,
>  			       enum expand_modifier, rtx *, bool);
>  extern rtx expand_expr_real_2 (sepops, rtx, machine_mode,
>  			       enum expand_modifier);
> +extern rtx expand_expr_real_gassign (gassign *, rtx, machine_mode,
> +				     enum expand_modifier modifier,
> +				     rtx * = nullptr, bool = false);
>  
>  /* Generate code for computing expression EXP.
>     An rtx for the computed value is returned.  The value is never null.
> diff --git a/gcc/testsuite/gcc.target/aarch64/ccmp_3.c b/gcc/testsuite/gcc.target/aarch64/ccmp_3.c
> new file mode 100644
> index 00000000000..a2b47fbee14
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/ccmp_3.c
> @@ -0,0 +1,20 @@
> +/* { dg-options "-O2" } */
> +/* PR target/100942 */
> +
> +void foo(void);
> +int f1(int a, int b)
> +{
> +  int c = a == 0 || b == 0;
> +  if (c) foo();
> +  return c;
> +}
> +
> +/* We should get one cmp followed by ccmp and one cset. */
> +/* { dg-final { scan-assembler "\tccmp\t" } } */
> +/* { dg-final { scan-assembler-times "\tcset\t" 1 } } */
> +/* { dg-final { scan-assembler-times "\tcmp\t" 1 } } */
> +/* And not get 2 cmps and 2 (or more cset) and orr and a cbnz. */
> +/* { dg-final { scan-assembler-not "\torr\t" } } */
> +/* { dg-final { scan-assembler-not "\tcbnz\t" } } */
> +/* { dg-final { scan-assembler-not "\tcbz\t" } } */
> +
> diff --git a/gcc/testsuite/gcc.target/aarch64/ccmp_4.c b/gcc/testsuite/gcc.target/aarch64/ccmp_4.c
> new file mode 100644
> index 00000000000..bc0f57a7c59
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/ccmp_4.c
> @@ -0,0 +1,35 @@
> +/* { dg-options "-O2" } */
> +/* PR target/100942 */
> +
> +void foo(void);
> +int f1(int a, int b, int d)
> +{
> +  int c = a < 8 || b < 9;
> +  int e = d < 11 || c;
> +  if (e) foo();
> +  return c;
> +}
> +
> +/*
> +  We really should get:
> +        cmp     w0, 7
> +        ccmp    w1, 8, 4, gt
> +        cset    w0, le
> +        ccmp    w2, 10, 4, gt
> +        ble     .L11
> +
> +  But we currently get:
> +        cmp     w0, 7
> +        ccmp    w1, 8, 4, gt
> +        cset    w0, le
> +        cmp     w0, 0
> +        ccmp    w2, 10, 4, eq
> +        ble     .L11
> +  The middle cmp is not needed.
> + */
> +
> +/* We should end up with only one cmp and 2 ccmp and 1 cset but currently we get 2 cmp
> +   though. */
> +/* { dg-final { scan-assembler-times "\tccmp\t" 2 } } */
> +/* { dg-final { scan-assembler-times "\tcset\t" 1 } } */
> +/* { dg-final { scan-assembler-times "\tcmp\t" 1 { xfail *-*-* } } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/ccmp_5.c b/gcc/testsuite/gcc.target/aarch64/ccmp_5.c
> new file mode 100644
> index 00000000000..7e52ae4f322
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/ccmp_5.c
> @@ -0,0 +1,20 @@
> +
> +/* { dg-options "-O2" } */
> +/* PR target/100942 */
> +void f1(int a, int b, _Bool *x)
> +{
> +  x[0] = x[1] = a == 0 || b == 0;
> +}
> +
> +void f2(int a, int b, int *x)
> +{
> +  x[0] = x[1] = a == 0 || b == 0;
> +}
> +
> +
> +/* Both functions should be using ccmp rather than 2 cset/orr.  */
> +/* { dg-final { scan-assembler-times "\tccmp\t" 2 } } */
> +/* { dg-final { scan-assembler-times "\tcset\t" 2 } } */
> +/* { dg-final { scan-assembler-times "\tcmp\t" 2 } } */
> +/* { dg-final { scan-assembler-not "\torr\t" } } */
> +