> Before, we had a loop with two iterations that tried "emit_cmov (cond, > op2, op3)". op2 and op3 can (or even were) If this did not succeed we > would revert the condition as well as op3 and op3 in-place and try > again. I found that a bit cumbersome and intended to make this explicit > but it's still kind of involved, particularly since cond may come in > reversed, we additionally swap op2 and op3 and all the way back again. > > I remember from the first iteration of these patches that this (double > revert) was needed for exactly one test case in the test suite on x86. > When re-running it today I could not reproduce it anymore, though.
Hmm, OK. Doesn't expanding both versions up-front create the same kind of problem that the patch is fixing, in that we expand (and therefore cost) both the reversed and unreversed comparison? Also… Robin Dapp <rd...@linux.ibm.com> writes: > @@ -4782,17 +4784,30 @@ emit_conditional_move (rtx target, enum rtx_code > code, rtx op0, rtx op1, > if (cmode == VOIDmode) > cmode = GET_MODE (op0); > > - enum rtx_code orig_code = code; > + /* If the first condition operand is constant and the second is not, swap > + them. In that case we also need to reverse the comparison. > + If both are non-constant It is possible that the conditional move > + will not expand with operands in this order, e.g. when we are about to > + create a min/max expression and the operands do not match the condition. > + In that case we also need the reversed condition code and comparison. > */ …for min/max, I would have expected this swap to create the canonical operand order for the min and max too. What causes it to be rejected? > + > + rtx rev_comparison = NULL_RTX; > bool swapped = false; > - if (swap_commutative_operands_p (op2, op3) > - && ((reversed = reversed_comparison_code_parts (code, op0, op1, NULL)) > - != UNKNOWN)) > + > + code = unsignedp ? unsigned_condition (code) : code; > + comparison = simplify_gen_relational (code, VOIDmode, cmode, op0, op1); > + > + if ((reversed = reversed_comparison_code_parts (code, op0, op1, NULL)) > + != UNKNOWN) > { > - std::swap (op2, op3); > - code = reversed; > - swapped = true; > + reversed = unsignedp ? unsigned_condition (reversed) : reversed; When is this needed? I'd have expected the reversed from of an unsigned code to be naturally unsigned. Thanks, Richard > + rev_comparison = simplify_gen_relational (reversed, VOIDmode, cmode, > + op0, op1); > } > > + if (swap_commutative_operands_p (op2, op3) && reversed != UNKNOWN) > + swapped = true; > + > if (mode == VOIDmode) > mode = GET_MODE (op2); > > @@ -4804,58 +4819,99 @@ emit_conditional_move (rtx target, enum rtx_code > code, rtx op0, rtx op1, > if (!target) > target = gen_reg_rtx (mode); > > - for (int pass = 0; ; pass++) > + if (comparison && COMPARISON_P (comparison)) > + prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), > + GET_CODE (comparison), NULL_RTX, > + unsignedp, OPTAB_WIDEN, &comparison, &cmode); > + else > + return NULL_RTX; > + > + if (rev_comparison && COMPARISON_P (rev_comparison)) > + prepare_cmp_insn (XEXP (rev_comparison, 0), XEXP (rev_comparison, 1), > + GET_CODE (rev_comparison), NULL_RTX, > + unsignedp, OPTAB_WIDEN, &rev_comparison, &cmode); > + > + if (!swapped) > + return emit_conditional_move (target, comparison, rev_comparison, > + op2, op3, mode); > + else > + return emit_conditional_move (target, rev_comparison, comparison, > + op3, op2, mode); > +} > + > +/* Helper function for emitting a conditional move. Given a COMPARISON > + and a reversed REV_COMPARISON it will try to expand a conditional move > + with COMPARISON first and try with REV_COMPARISON if that fails. */ > + > +rtx > +emit_conditional_move (rtx target, rtx comparison, rtx rev_comparison, > + rtx op2, rtx op3, machine_mode mode) > +{ > + > + rtx res = emit_conditional_move (target, comparison, op2, op3, mode); > + > + if (res != NULL_RTX) > + return res; > + > + return emit_conditional_move (target, rev_comparison, op3, op2, mode); > +} > + > +/* Helper for emitting a conditional move. */ > + > +static rtx > +emit_conditional_move (rtx target, rtx comparison, > + rtx op2, rtx op3, machine_mode mode) > +{ > + rtx_insn *last; > + enum insn_code icode; > + > + if (comparison == NULL_RTX || !COMPARISON_P (comparison)) > + return NULL_RTX; > + > + /* If the two source operands are identical, that's just a move. */ > + if (rtx_equal_p (op2, op3)) > { > - code = unsignedp ? unsigned_condition (code) : code; > - comparison = simplify_gen_relational (code, VOIDmode, cmode, op0, op1); > + if (!target) > + target = gen_reg_rtx (mode); > > - /* We can get const0_rtx or const_true_rtx in some circumstances. Just > - punt and let the caller figure out how best to deal with this > - situation. */ > - if (COMPARISON_P (comparison)) > - { > - saved_pending_stack_adjust save; > - save_pending_stack_adjust (&save); > - last = get_last_insn (); > - do_pending_stack_adjust (); > - machine_mode cmpmode = cmode; > - prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), > - GET_CODE (comparison), NULL_RTX, unsignedp, > - OPTAB_WIDEN, &comparison, &cmpmode); > - if (comparison) > - { > - class expand_operand ops[4]; > + emit_move_insn (target, op3); > + return target; > + } > > - create_output_operand (&ops[0], target, mode); > - create_fixed_operand (&ops[1], comparison); > - create_input_operand (&ops[2], op2, mode); > - create_input_operand (&ops[3], op3, mode); > - if (maybe_expand_insn (icode, 4, ops)) > - { > - if (ops[0].value != target) > - convert_move (target, ops[0].value, false); > - return target; > - } > - } > - delete_insns_since (last); > - restore_pending_stack_adjust (&save); > - } > + if (mode == VOIDmode) > + mode = GET_MODE (op2); > > - if (pass == 1) > - return NULL_RTX; > + icode = direct_optab_handler (movcc_optab, mode); > > - /* If the preferred op2/op3 order is not usable, retry with other > - operand order, perhaps it will expand successfully. */ > - if (swapped) > - code = orig_code; > - else if ((reversed = reversed_comparison_code_parts (orig_code, op0, > op1, > - NULL)) > - != UNKNOWN) > - code = reversed; > - else > - return NULL_RTX; > - std::swap (op2, op3); > + if (icode == CODE_FOR_nothing) > + return NULL_RTX; > + > + if (!target) > + target = gen_reg_rtx (mode); > + > + saved_pending_stack_adjust save; > + save_pending_stack_adjust (&save); > + last = get_last_insn (); > + do_pending_stack_adjust (); > + > + class expand_operand ops[4]; > + > + create_output_operand (&ops[0], target, mode); > + create_fixed_operand (&ops[1], comparison); > + create_input_operand (&ops[2], op2, mode); > + create_input_operand (&ops[3], op3, mode); > + > + if (maybe_expand_insn (icode, 4, ops)) > + { > + if (ops[0].value != target) > + convert_move (target, ops[0].value, false); > + return target; > } > + > + delete_insns_since (last); > + restore_pending_stack_adjust (&save); > + > + return NULL_RTX; > } > > > diff --git a/gcc/optabs.h b/gcc/optabs.h > index 3bbceff92d9..f853b93f37f 100644 > --- a/gcc/optabs.h > +++ b/gcc/optabs.h > @@ -281,6 +281,7 @@ extern void emit_indirect_jump (rtx); > /* Emit a conditional move operation. */ > rtx emit_conditional_move (rtx, enum rtx_code, rtx, rtx, machine_mode, > rtx, rtx, machine_mode, int); > +rtx emit_conditional_move (rtx, rtx, rtx, rtx, rtx, machine_mode); > > /* Emit a conditional negate or bitwise complement operation. */ > rtx emit_conditional_neg_or_complement (rtx, rtx_code, machine_mode, rtx,