Sorry for the slow review. Robin Dapp <rd...@linux.ibm.com> writes: > When if-converting multiple SETs and we encounter a swap-style idiom > > if (a > b) > { > tmp = c; // [1] > c = d; > d = tmp; > } > > ifcvt should not generate a conditional move for the instruction at > [1]. > > In order to achieve that, this patch goes through all relevant SETs > and marks the relevant instructions. This helps to evaluate costs. > > On top, only generate temporaries if the current cmov is going to > overwrite one of the comparands of the initial compare. > --- > gcc/ifcvt.c | 104 +++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 87 insertions(+), 17 deletions(-) > > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c > index 017944f4f79..eef6490626a 100644 > --- a/gcc/ifcvt.c > +++ b/gcc/ifcvt.c > @@ -98,6 +98,7 @@ static int dead_or_predicable (basic_block, basic_block, > basic_block, > edge, int); > static void noce_emit_move_insn (rtx, rtx); > static rtx_insn *block_has_only_trap (basic_block); > +static void check_need_cmovs (basic_block, hash_map<rtx, bool> *); > > /* Count the number of non-jump active insns in BB. */ > > @@ -3203,6 +3204,10 @@ noce_convert_multiple_sets (struct noce_if_info > *if_info) > auto_vec<rtx_insn *> unmodified_insns; > int count = 0; > > + hash_map<rtx, bool> need_cmovs;
A hash_set might be simpler, given that the code only enters insns for which the bool is false. “rtx_insn *” would be better than rtx. > + > + check_need_cmovs (then_bb, &need_cmovs); > + > FOR_BB_INSNS (then_bb, insn) > { > /* Skip over non-insns. */ > @@ -3213,26 +3218,38 @@ noce_convert_multiple_sets (struct noce_if_info > *if_info) > gcc_checking_assert (set); > > rtx target = SET_DEST (set); > - rtx temp = gen_reg_rtx (GET_MODE (target)); > + rtx temp; > rtx new_val = SET_SRC (set); > rtx old_val = target; > > - /* If we were supposed to read from an earlier write in this block, > - we've changed the register allocation. Rewire the read. While > - we are looking, also try to catch a swap idiom. */ > - for (int i = count - 1; i >= 0; --i) > - if (reg_overlap_mentioned_p (new_val, targets[i])) > - { > - /* Catch a "swap" style idiom. */ > - if (find_reg_note (insn, REG_DEAD, new_val) != NULL_RTX) > - /* The write to targets[i] is only live until the read > - here. As the condition codes match, we can propagate > - the set to here. */ > - new_val = SET_SRC (single_set (unmodified_insns[i])); > - else > - new_val = temporaries[i]; > - break; > - } > + /* As we are transforming > + if (x > y) > + a = b; > + c = d; Looks like some missing braces here. > + into > + a = (x > y) ... > + c = (x > y) ... > + > + we potentially check x > y before every set here. > + (Even though might be removed by subsequent passes.) Do you mean the sets might be removed or that the checks might be removed? > + We cannot transform > + if (x > y) > + x = y; > + ... > + into > + x = (x > y) ... > + ... > + since this would invalidate x. Therefore we introduce a temporary > + every time we are about to overwrite a variable used in the > + check. Costing of a sequence with these is going to be inaccurate. */ > + if (reg_overlap_mentioned_p (target, cond)) > + temp = gen_reg_rtx (GET_MODE (target)); > + else > + temp = target; > + > + bool need_cmov = true; > + if (need_cmovs.get (insn)) > + need_cmov = false; The patch is quite hard to review on its own, since nothing actually uses this variable. It's also not obvious how the reg_overlap_mentioned_p code works if the old target is referenced later. Could you refactor the series a bit so that each patch is self-contained? It's OK if that means fewer patches. Thanks, Richard > /* If we had a non-canonical conditional jump (i.e. one where > the fallthrough is to the "else" case) we need to reverse > @@ -3808,6 +3825,59 @@ check_cond_move_block (basic_block bb, > return TRUE; > } > > +/* Find local swap-style idioms in BB and mark the first insn (1) > + that is only a temporary as not needing a conditional move as > + it is going to be dead afterwards anyway. > + > + (1) int tmp = a; > + a = b; > + b = tmp; > + > + ifcvt > + --> > + > + load tmp,a > + cmov a,b > + cmov b,tmp */ > + > +static void > +check_need_cmovs (basic_block bb, hash_map<rtx, bool> *need_cmov) > +{ > + rtx_insn *insn; > + int count = 0; > + auto_vec<rtx> insns; > + auto_vec<rtx> dests; > + > + FOR_BB_INSNS (bb, insn) > + { > + rtx set, src, dest; > + > + if (!active_insn_p (insn)) > + continue; > + > + set = single_set (insn); > + if (set == NULL_RTX) > + continue; > + > + src = SET_SRC (set); > + dest = SET_DEST (set); > + > + for (int i = count - 1; i >= 0; --i) > + { > + if (reg_overlap_mentioned_p (src, dests[i]) > + && find_reg_note (insn, REG_DEAD, src) != NULL_RTX) > + { > + need_cmov->put (insns[i], false); > + } > + } > + > + insns.safe_push (insn); > + dests.safe_push (dest); > + > + count++; > + } > +} > + > /* Given a basic block BB suitable for conditional move conversion, > a condition COND, and pointer maps THEN_VALS and ELSE_VALS containing > the register values depending on COND, emit the insns in the block as