From: Richard Sandiford <richard.sandiford@arm.com>
To: Robin Dapp <rdapp@linux.ibm.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH 4/7] ifcvt/optabs: Allow using a CC comparison for emit_conditional_move.
Date: Mon, 26 Jul 2021 20:31:57 +0100 [thread overview]
Message-ID: <mpt1r7kna6q.fsf@arm.com> (raw)
In-Reply-To: <06f8a450-4da1-b7e3-4b5b-e7085cfa822a@linux.ibm.com> (Robin Dapp's message of "Thu, 22 Jul 2021 14:07:18 +0200")
> 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 <rdapp@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,
next prev parent reply other threads:[~2021-07-26 19:31 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-25 16:08 [PATCH 0/7] ifcvt: Convert multiple Robin Dapp
2021-06-25 16:08 ` [PATCH 1/7] ifcvt: Check if cmovs are needed Robin Dapp
2021-07-15 20:10 ` Richard Sandiford
2021-07-22 12:06 ` Robin Dapp
2021-07-26 19:08 ` Richard Sandiford
2021-09-15 8:39 ` Robin Dapp
2021-10-14 8:45 ` Richard Sandiford
2021-10-14 14:20 ` Robin Dapp
2021-10-14 14:32 ` Richard Sandiford
2021-10-18 11:40 ` Robin Dapp
2021-11-03 8:55 ` Robin Dapp
2021-11-05 15:33 ` Richard Sandiford
2021-11-12 13:00 ` Robin Dapp
2021-11-30 16:36 ` Richard Sandiford
2021-06-25 16:09 ` [PATCH 2/7] ifcvt: Allow constants for noce_convert_multiple Robin Dapp
2021-07-15 20:25 ` Richard Sandiford
2021-06-25 16:09 ` [PATCH 3/7] ifcvt: Improve costs handling " Robin Dapp
2021-07-15 20:42 ` Richard Sandiford
2021-07-22 12:07 ` Robin Dapp
2021-07-26 19:10 ` Richard Sandiford
2021-06-25 16:09 ` [PATCH 4/7] ifcvt/optabs: Allow using a CC comparison for emit_conditional_move Robin Dapp
2021-07-15 20:54 ` Richard Sandiford
2021-07-22 12:07 ` Robin Dapp
2021-07-26 19:31 ` Richard Sandiford [this message]
2021-07-27 20:49 ` Robin Dapp
2021-08-06 12:14 ` Richard Sandiford
2021-06-25 16:09 ` [PATCH 5/7] ifcvt: Try re-using CC for conditional moves Robin Dapp
2021-07-22 12:12 ` Robin Dapp
2021-06-25 16:09 ` [PATCH 6/7] testsuite/s390: Add tests for noce_convert_multiple Robin Dapp
2021-06-25 16:09 ` [PATCH 7/7] s390: Increase costs for load on condition and change movqicc expander Robin Dapp
2021-07-13 12:42 ` [PATCH 0/7] ifcvt: Convert multiple Robin Dapp
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mpt1r7kna6q.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=rdapp@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).