Robin Dapp <rdapp....@gmail.com> writes:
> The attached v3 tracks the use of cond_earliest as you suggested
> and adds its cost in default_noce_conversion_profitable_p.
>
> Bootstrapped and regtested on x86 and p10, aarch64 still
> running. Regtested on riscv64.
>
> Regards
> Robin
>
> Before noce_find_if_block processes a block it sets up an if_info
> structure that holds the original costs. At that point the costs of
> the then/else blocks have not been added so we only care about the
> "if" cost.
>
> The code originally used BRANCH_COST for that but was then changed
> to COST_N_INSNS (2) - a compare and a jump.
>
> This patch computes the jump costs via
> insn_cost (if_info.jump, ...)
> under the assumption that the target takes BRANCH_COST into account
> when costing a jump instruction.
>
> In noce_convert_multiple_sets, we keep track of the need for the initial
> CC comparison. If we needed it for the generated sequence we add its
> cost in default_noce_conversion_profitable_p.
I was looking at the code in more detail and just wanted to check.
We have:
int last_needs_comparison = -1;
bool ok = noce_convert_multiple_sets_1
(if_info, &need_no_cmov, &rewired_src, &targets, &temporaries,
&unmodified_insns, &last_needs_comparison);
if (!ok)
return false;
/* If there are insns that overwrite part of the initial
comparison, we can still omit creating temporaries for
the last of them.
As the second try will always create a less expensive,
valid sequence, we do not need to compare and can discard
the first one. */
if (last_needs_comparison != -1)
{
end_sequence ();
start_sequence ();
ok = noce_convert_multiple_sets_1
(if_info, &need_no_cmov, &rewired_src, &targets, &temporaries,
&unmodified_insns, &last_needs_comparison);
/* Actually we should not fail anymore if we reached here,
but better still check. */
if (!ok)
return false;
}
But noce_convert_multiple_sets_1 ends with:
/* Even if we did not actually need the comparison, we want to make sure
to try a second time in order to get rid of the temporaries. */
if (*last_needs_comparison == -1)
*last_needs_comparison = 0;
return true;
AFAICT that means that the first attempt is always redundant.
Have I missed something?
I don't know if this was something that Manolis's patches addressed.
Thanks,
Richard
>
> gcc/ChangeLog:
>
> * ifcvt.cc (default_noce_conversion_profitable_p): Add cost of
> CC comparison.
> (noce_convert_multiple_sets_1): Set use_cond_earliest.
> (noce_process_if_block): Just use original cost.
> (noce_find_if_block): Use insn_cost (jump_insn).
> * ifcvt.h (struct noce_if_info): Add use_cond_earliest.
> ---
> gcc/ifcvt.cc | 37 ++++++++++++++++++++++---------------
> gcc/ifcvt.h | 3 +++
> 2 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> index 58ed42673e5..9b408eeb313 100644
> --- a/gcc/ifcvt.cc
> +++ b/gcc/ifcvt.cc
> @@ -814,7 +814,16 @@ default_noce_conversion_profitable_p (rtx_insn *seq,
> /* Cost up the new sequence. */
> unsigned int cost = seq_cost (seq, speed_p);
>
> - if (cost <= if_info->original_cost)
> + /* If the created sequence does not use cond_earliest (but the jump
> + does) add its cost to the original_cost here. */
> + unsigned int cost_adjust = 0;
> +
> + if (if_info->jump != if_info->cond_earliest
> + && !if_info->use_cond_earliest)
> + cost_adjust = insn_cost (if_info->cond_earliest,
> + if_info->speed_p);
> +
> + if (cost <= if_info->original_cost + cost_adjust)
> return true;
>
> /* When compiling for size, we can make a reasonably accurately guess
> @@ -3780,6 +3789,7 @@ noce_convert_multiple_sets_1 (struct noce_if_info
> *if_info,
> temp_dest = temp_dest2;
> if (!second_try && read_comparison)
> *last_needs_comparison = count;
> + if_info->use_cond_earliest = true;
> }
> else
> {
> @@ -3931,16 +3941,13 @@ noce_process_if_block (struct noce_if_info *if_info)
> to calculate a value for x.
> ??? For future expansion, further expand the "multiple X" rules. */
>
> - /* First look for multiple SETS. The original costs already include
> - a base cost of COSTS_N_INSNS (2): one instruction for the compare
> - (which we will be needing either way) and one instruction for the
> - branch. When comparing costs we want to use the branch instruction
> - cost and the sets vs. the cmovs generated here. Therefore subtract
> - the costs of the compare before checking.
> - ??? Actually, instead of the branch instruction costs we might want
> - to use COSTS_N_INSNS (BRANCH_COST ()) as in other places. */
> + /* First look for multiple SETS.
> + The original costs already include costs for the jump insn as well
> + as for a CC comparison if there is any.
> + If a target re-uses the existing CC comparison we keep track of that
> + and add the costs in default default_noce_conversion_profitable_p. */
>
> - unsigned potential_cost = if_info->original_cost - COSTS_N_INSNS (1);
> + unsigned potential_cost = if_info->original_cost;
> unsigned old_cost = if_info->original_cost;
> if (!else_bb
> && HAVE_conditional_move
> @@ -4696,6 +4703,7 @@ noce_find_if_block (basic_block test_bb, edge
> then_edge, edge else_edge,
> gcc_assert (if_info.rev_cond == NULL_RTX
> || rev_cond_earliest == cond_earliest);
> if_info.cond_earliest = cond_earliest;
> + if_info.use_cond_earliest = false;
> if_info.jump = jump;
> if_info.then_else_reversed = then_else_reversed;
> if_info.speed_p = speed_p;
> @@ -4703,11 +4711,10 @@ noce_find_if_block (basic_block test_bb, edge
> then_edge, edge else_edge,
> = targetm.max_noce_ifcvt_seq_cost (then_edge);
> /* We'll add in the cost of THEN_BB and ELSE_BB later, when we check
> that they are valid to transform. We can't easily get back to the insn
> - for COND (and it may not exist if we had to canonicalize to get COND),
> - and jump_insns are always given a cost of 1 by seq_cost, so treat
> - both instructions as having cost COSTS_N_INSNS (1). */
> - if_info.original_cost = COSTS_N_INSNS (2);
> -
> + for COND (and it may not exist if we had to canonicalize to get COND).
> + It is assumed that the costs of a jump insn are dependent on the
> + branch costs. */
> + if_info.original_cost += insn_cost (if_info.jump, if_info.speed_p);
>
> /* Do the real work. */
>
> diff --git a/gcc/ifcvt.h b/gcc/ifcvt.h
> index 37147f99129..67d4f6a502e 100644
> --- a/gcc/ifcvt.h
> +++ b/gcc/ifcvt.h
> @@ -63,6 +63,9 @@ struct noce_if_info
> /* New insns should be inserted before this one. */
> rtx_insn *cond_earliest;
>
> + /* Whether or not COND_EARLIEST is used in the resulting sequence. */
> + bool use_cond_earliest;
> +
> /* Insns in the THEN and ELSE block. There is always just this
> one insns in those blocks. The insns are single_set insns.
> If there was no ELSE block, INSN_B is the last insn before
