Robin Dapp <rdapp....@gmail.com> writes:
>> Is there any way we can avoid using pattern_cost here? Using it means
>> that we can make use of targetm.insn_cost for the jump but circumvent
>> it for the condition, giving a bit of a mixed metric.
>>
>> (I realise there are existing calls to pattern_cost in ifcvt.cc,
>> but if possible I think we should try to avoid adding more.)
>
> Yes, I believe there is. In addition, what I did with
> if_info->cond wasn't what I intended to do.
>
> The whole point of the exercise is that noce_convert_multiple_sets
> can re-use the CC comparison that is already present (because it
> is used in the jump pattern). Therefore I want to split costs
> into a jump part and a CC-setting part so the final costing
> decision for multiple sets can be:
>
> insn_cost (jump) + n * insn_cost (set)
> vs
> n * insn_cost ("cmov")
>
> Still, the original costs should be:
> insn_cost (set_cc) + insn_cost (jump)
> and with the split we can just remove insn_cost (set_cc) before
> the multiple-set cost comparison and re-add it afterwards.
>
> For non-CC targets this is not necessary.
>
> So what I'd hope is better is to use
> insn_cost (if_info.earliest_cond)
> which is indeed the CC-set/comparison if it exists.
I agree that's probably good enough in practice. It doesn't cope
with things like:
/* Handle sequences like:
(set op0 (xor X Y))
...(eq|ne op0 (const_int 0))...
in which case:
(eq op0 (const_int 0)) reduces to (eq X Y)
(ne op0 (const_int 0)) reduces to (ne X Y)
This is the form used by MIPS16, for example. */
but then neither does the current code. But...
> The attached v2 was bootstrapped and regtested on x86, aarch64 and
> power10 and regtested on riscv64.
>
> Regards
> Robin
>
> gcc/ChangeLog:
>
> * ifcvt.cc (noce_process_if_block): Subtract condition pattern
> cost if applicable.
> (noce_find_if_block): Use insn_cost and pattern_cost for
> original cost.
> ---
> gcc/ifcvt.cc | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> index 58ed42673e5..ebb838fd82c 100644
> --- a/gcc/ifcvt.cc
> +++ b/gcc/ifcvt.cc
> @@ -3931,16 +3931,16 @@ 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. */
> -
> - unsigned potential_cost = if_info->original_cost - COSTS_N_INSNS (1);
> + /* 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.
> + We want to allow the backend to re-use the existing CC comparison
> + and therefore don't consider it for the cost comparison (as it is
> + then needed for both the jump as well as the cmov sequence). */
> +
> + unsigned potential_cost = if_info->original_cost;
> + if (if_info->cond_earliest && if_info->jump != if_info->cond_earliest)
> + potential_cost -= insn_cost (if_info->cond_earliest, if_info->speed_p);
> unsigned old_cost = if_info->original_cost;
> if (!else_bb
> && HAVE_conditional_move
...why do we do the adjustment here? Doesn't noce_convert_multiple_sets_1
know for certain (or at least with more certainty) whether any of the
new instructions use the old CC result? It seems like we could record
that and do the adjustment around the call to
targetm.noce_conversion_profitable_p.
> @@ -4703,11 +4703,12 @@ 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).
> + jump insn that is costed via insn_cost. It is assumed that the
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Looks like this part of the comment got a bit garbled.
Thanks,
Richard
> + costs of a jump insn are dependent on the branch costs. */
> + if (if_info.cond_earliest && if_info.jump != if_info.cond_earliest)
> + if_info.original_cost = insn_cost (if_info.cond_earliest,
> if_info.speed_p);
> + if_info.original_cost += insn_cost (if_info.jump, if_info.speed_p);
>
> /* Do the real work. */
