> 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. 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 @@ -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 + 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. */ -- 2.45.1