Robin Dapp <rdapp....@gmail.com> writes: > Hi, > > 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, ...) > which is supposed to incorporate the branch costs and, in case of a CC > comparison, > pattern_cost (if_info.cond, ...) > which is supposed to account for the CC creation. > > For compare_and_jump patterns insn_cost should have already computed > the right cost. > > Does this "split" make sense, generally? > > Bootstrapped and regtested on x86, aarch64 and power10. Regtested > on riscv. > > 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 | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc > index 58ed42673e5..305b9faed38 100644 > --- a/gcc/ifcvt.cc > +++ b/gcc/ifcvt.cc > @@ -3940,7 +3940,9 @@ noce_process_if_block (struct noce_if_info *if_info) > ??? 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); > + unsigned potential_cost = if_info->original_cost; > + if (cc_in_cond (if_info->cond)) > + potential_cost -= pattern_cost (if_info->cond, if_info->speed_p); > unsigned old_cost = if_info->original_cost; > if (!else_bb > && HAVE_conditional_move > @@ -4703,11 +4705,13 @@ 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). > + Here we assume one CC compare insn (if the target uses CC) and one > + 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 (cc_in_cond (if_info.cond)) > + if_info.original_cost = pattern_cost (if_info.cond, if_info.speed_p); > + if_info.original_cost += insn_cost (if_info.jump, if_info.speed_p); > > /* Do the real work. */
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.) Thanks, Richard
