Re: [PATCH 4/7] ifcvt/optabs: Allow using a CC comparison for emit_conditional_move.

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

Robin Dapp <rdapp@linux.ibm.com> writes:
> Currently we only ever call emit_conditional_move with the comparison
> (as well as its comparands) we got from the jump.  Thus, backends are
> going to emit a CC comparison for every conditional move that is being
> generated instead of re-using the existing CC.
> This, combined with emitting temporaries for each conditional move,
> causes sky-high costs for conditional moves.
>
> This patch allows to re-use a CC so the costing situation is improved a
> bit.
> ---
>  gcc/ifcvt.c  |  16 +++--
>  gcc/optabs.c | 163 ++++++++++++++++++++++++++++++++++-----------------
>  gcc/optabs.h |   1 +
>  3 files changed, 121 insertions(+), 59 deletions(-)
>
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index ac0c142c9fe..c5b8641e2aa 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -771,7 +771,7 @@ static int noce_try_addcc (struct noce_if_info *);
>  static int noce_try_store_flag_constants (struct noce_if_info *);
>  static int noce_try_store_flag_mask (struct noce_if_info *);
>  static rtx noce_emit_cmove (struct noce_if_info *, rtx, enum rtx_code, rtx,
> -			    rtx, rtx, rtx);
> +			    rtx, rtx, rtx, rtx = NULL, rtx = NULL);
>  static int noce_try_cmove (struct noce_if_info *);
>  static int noce_try_cmove_arith (struct noce_if_info *);
>  static rtx noce_get_alt_condition (struct noce_if_info *, rtx, rtx_insn **);
> @@ -1710,7 +1710,8 @@ noce_try_store_flag_mask (struct noce_if_info *if_info)
>  
>  static rtx
>  noce_emit_cmove (struct noce_if_info *if_info, rtx x, enum rtx_code code,
> -		 rtx cmp_a, rtx cmp_b, rtx vfalse, rtx vtrue)
> +		 rtx cmp_a, rtx cmp_b, rtx vfalse, rtx vtrue, rtx cc_cmp,
> +		 rtx rev_cc_cmp)
>  {
>    rtx target ATTRIBUTE_UNUSED;
>    int unsignedp ATTRIBUTE_UNUSED;
> @@ -1756,9 +1757,14 @@ noce_emit_cmove (struct noce_if_info *if_info, rtx x, enum rtx_code code,
>    unsignedp = (code == LTU || code == GEU
>  	       || code == LEU || code == GTU);
>  
> -  target = emit_conditional_move (x, code, cmp_a, cmp_b, VOIDmode,
> -				  vtrue, vfalse, GET_MODE (x),
> -				  unsignedp);
> +  if (cc_cmp != NULL_RTX && rev_cc_cmp != NULL_RTX)
> +    target = emit_conditional_move (x, cc_cmp, rev_cc_cmp,
> +				    vtrue, vfalse, GET_MODE (x));
> +  else
> +    target = emit_conditional_move (x, code, cmp_a, cmp_b, VOIDmode,
> +				    vtrue, vfalse, GET_MODE (x),
> +				    unsignedp);

It might make sense to move:

  /* Don't even try if the comparison operands are weird
     except that the target supports cbranchcc4.  */
  if (! general_operand (cmp_a, GET_MODE (cmp_a))
      || ! general_operand (cmp_b, GET_MODE (cmp_b)))
    {
      if (!have_cbranchcc4
	  || GET_MODE_CLASS (GET_MODE (cmp_a)) != MODE_CC
	  || cmp_b != const0_rtx)
	return NULL_RTX;
    }

into the “else” arm, since it seems odd to be checking cmp_a and cmp_b
when we're not going to use them.  Looks like the later call to
emit_conditional_move should get the same treatment.

> +
>    if (target)
>      return target;
>  
> diff --git a/gcc/optabs.c b/gcc/optabs.c
> index 62a6bdb4c59..6bf486b9b50 100644
> --- a/gcc/optabs.c
> +++ b/gcc/optabs.c
> @@ -52,6 +52,8 @@ static void prepare_float_lib_cmp (rtx, rtx, enum rtx_code, rtx *,
>  static rtx expand_unop_direct (machine_mode, optab, rtx, rtx, int);
>  static void emit_libcall_block_1 (rtx_insn *, rtx, rtx, rtx, bool);
>  
> +static rtx emit_conditional_move (rtx, rtx, rtx, rtx, machine_mode);
> +
>  /* Debug facility for use in GDB.  */
>  void debug_optab_libfuncs (void);
>  \f
> @@ -4747,7 +4749,6 @@ emit_conditional_move (rtx target, enum rtx_code code, rtx op0, rtx op1,
>  		       machine_mode mode, int unsignedp)
>  {
>    rtx comparison;
> -  rtx_insn *last;
>    enum insn_code icode;
>    enum rtx_code reversed;
>  
> @@ -4774,6 +4775,7 @@ emit_conditional_move (rtx target, enum rtx_code code, rtx op0, rtx op1,
>    /* get_condition will prefer to generate LT and GT even if the old
>       comparison was against zero, so undo that canonicalization here since
>       comparisons against zero are cheaper.  */
> +
>    if (code == LT && op1 == const1_rtx)
>      code = LE, op1 = const0_rtx;
>    else if (code == GT && op1 == constm1_rtx)
> @@ -4782,17 +4784,29 @@ emit_conditional_move (rtx target, enum rtx_code code, rtx op0, rtx op1,
>    if (cmode == VOIDmode)
>      cmode = GET_MODE (op0);
>  
> -  enum rtx_code orig_code = code;
> +  /* If the first source operand is constant and the second is not, swap
> +     it into the second.  In that case we also need to reverse the
> +     comparison.  It is possible, though, that the conditional move
> +     will not expand with operands in this order, so we might also need
> +     to revert to the original comparison and operand order.  */

Why's that the case though?  The swapped form is the canonical one,
so it's the one that the target ought to accept.

Thanks,
Richard

> +
> +  rtx rev_comparison = NULL_RTX;
>    bool swapped = false;
> -  if (swap_commutative_operands_p (op2, op3)
> -      && ((reversed = reversed_comparison_code_parts (code, op0, op1, NULL))
> -          != UNKNOWN))
> +
> +  code = unsignedp ? unsigned_condition (code) : code;
> +  comparison = simplify_gen_relational (code, VOIDmode, cmode, op0, op1);
> +
> +  if ((reversed = reversed_comparison_code_parts (code, op0, op1, NULL))
> +      != UNKNOWN)
>      {
> -      std::swap (op2, op3);
> -      code = reversed;
> -      swapped = true;
> +      reversed = unsignedp ? unsigned_condition (reversed) : reversed;
> +      rev_comparison = simplify_gen_relational (reversed, VOIDmode, cmode,
> +						op0, op1);
>      }
>  
> +  if (swap_commutative_operands_p (op2, op3) && reversed != UNKNOWN)
> +    swapped = true;
> +
>    if (mode == VOIDmode)
>      mode = GET_MODE (op2);
>  
> @@ -4804,58 +4818,99 @@ emit_conditional_move (rtx target, enum rtx_code code, rtx op0, rtx op1,
>    if (!target)
>      target = gen_reg_rtx (mode);
>  
> -  for (int pass = 0; ; pass++)
> +  if (comparison && COMPARISON_P (comparison))
> +    prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1),
> +		      GET_CODE (comparison), NULL_RTX, unsignedp, OPTAB_WIDEN,
> +		      &comparison, &cmode);
> +  else
> +    return NULL_RTX;
> +
> +  if (rev_comparison && COMPARISON_P (rev_comparison))
> +    prepare_cmp_insn (XEXP (rev_comparison, 0), XEXP (rev_comparison, 1),
> +		      GET_CODE (rev_comparison), NULL_RTX,
> +		      unsignedp, OPTAB_WIDEN, &rev_comparison, &cmode);
> +
> +  if (!swapped)
> +    return emit_conditional_move (target, comparison, rev_comparison,
> +				  op2, op3, mode);
> +  else
> +    return emit_conditional_move (target, rev_comparison, comparison,
> +				  op3, op2, mode);
> +}
> +
> +/* Helper function for emitting a conditional move.  Given a COMPARISON
> +   and a reversed REV_COMPARISON it will try to expand a conditional move
> +   with COMPARISON first and try with REV_COMPARISON if that fails.  */
> +
> +rtx
> +emit_conditional_move (rtx target, rtx comparison, rtx rev_comparison,
> +		       rtx op2, rtx op3, machine_mode mode)
> +{
> +
> +  rtx res = emit_conditional_move (target, comparison, op2, op3, mode);
> +
> +  if (res != NULL_RTX)
> +    return res;
> +
> +  return emit_conditional_move (target, rev_comparison, op3, op2, mode);
> +}
> +
> +/* Helper for emitting a conditional move.  */
> +
> +static rtx
> +emit_conditional_move (rtx target, rtx comparison,
> +		       rtx op2, rtx op3, machine_mode mode)
> +{
> +  rtx_insn *last;
> +  enum insn_code icode;
> +
> +  if (comparison == NULL_RTX || !COMPARISON_P (comparison))
> +    return NULL_RTX;
> +
> +  /* If the two source operands are identical, that's just a move.  */
> +  if (rtx_equal_p (op2, op3))
>      {
> -      code = unsignedp ? unsigned_condition (code) : code;
> -      comparison = simplify_gen_relational (code, VOIDmode, cmode, op0, op1);
> +      if (!target)
> +	target = gen_reg_rtx (mode);
>  
> -      /* We can get const0_rtx or const_true_rtx in some circumstances.  Just
> -	 punt and let the caller figure out how best to deal with this
> -	 situation.  */
> -      if (COMPARISON_P (comparison))
> -	{
> -	  saved_pending_stack_adjust save;
> -	  save_pending_stack_adjust (&save);
> -	  last = get_last_insn ();
> -	  do_pending_stack_adjust ();
> -	  machine_mode cmpmode = cmode;
> -	  prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1),
> -			    GET_CODE (comparison), NULL_RTX, unsignedp,
> -			    OPTAB_WIDEN, &comparison, &cmpmode);
> -	  if (comparison)
> -	    {
> -	      class expand_operand ops[4];
> +      emit_move_insn (target, op3);
> +      return target;
> +    }
>  
> -	      create_output_operand (&ops[0], target, mode);
> -	      create_fixed_operand (&ops[1], comparison);
> -	      create_input_operand (&ops[2], op2, mode);
> -	      create_input_operand (&ops[3], op3, mode);
> -	      if (maybe_expand_insn (icode, 4, ops))
> -		{
> -		  if (ops[0].value != target)
> -		    convert_move (target, ops[0].value, false);
> -		  return target;
> -		}
> -	    }
> -	  delete_insns_since (last);
> -	  restore_pending_stack_adjust (&save);
> -	}
> +  if (mode == VOIDmode)
> +    mode = GET_MODE (op2);
>  
> -      if (pass == 1)
> -	return NULL_RTX;
> +  icode = direct_optab_handler (movcc_optab, mode);
>  
> -      /* If the preferred op2/op3 order is not usable, retry with other
> -	 operand order, perhaps it will expand successfully.  */
> -      if (swapped)
> -	code = orig_code;
> -      else if ((reversed = reversed_comparison_code_parts (orig_code, op0, op1,
> -							   NULL))
> -	       != UNKNOWN)
> -	code = reversed;
> -      else
> -	return NULL_RTX;
> -      std::swap (op2, op3);
> +  if (icode == CODE_FOR_nothing)
> +    return NULL_RTX;
> +
> +  if (!target)
> +    target = gen_reg_rtx (mode);
> +
> +  saved_pending_stack_adjust save;
> +  save_pending_stack_adjust (&save);
> +  last = get_last_insn ();
> +  do_pending_stack_adjust ();
> +
> +  class expand_operand ops[4];
> +
> +  create_output_operand (&ops[0], target, mode);
> +  create_fixed_operand (&ops[1], comparison);
> +  create_input_operand (&ops[2], op2, mode);
> +  create_input_operand (&ops[3], op3, mode);
> +
> +  if (maybe_expand_insn (icode, 4, ops))
> +    {
> +      if (ops[0].value != target)
> +	convert_move (target, ops[0].value, false);
> +      return target;
>      }
> +
> +  delete_insns_since (last);
> +  restore_pending_stack_adjust (&save);
> +
> +  return NULL_RTX;
>  }
>  
>  
> diff --git a/gcc/optabs.h b/gcc/optabs.h
> index 3bbceff92d9..f853b93f37f 100644
> --- a/gcc/optabs.h
> +++ b/gcc/optabs.h
> @@ -281,6 +281,7 @@ extern void emit_indirect_jump (rtx);
>  /* Emit a conditional move operation.  */
>  rtx emit_conditional_move (rtx, enum rtx_code, rtx, rtx, machine_mode,
>  			   rtx, rtx, machine_mode, int);
> +rtx emit_conditional_move (rtx, rtx, rtx, rtx, rtx, machine_mode);
>  
>  /* Emit a conditional negate or bitwise complement operation.  */
>  rtx emit_conditional_neg_or_complement (rtx, rtx_code, machine_mode, rtx,