From: Richard Sandiford <richard.sandiford@arm.com>
To: Robin Dapp <rdapp@linux.ibm.com>
Cc: gcc-patches@gcc.gnu.org, james.greenhalgh@arm.com
Subject: Re: [PATCH 4/9] ifcvt: Estimate original costs before convert_multiple.
Date: Tue, 06 Aug 2019 20:30:00 -0000 [thread overview]
Message-ID: <mptmugm6ogj.fsf@arm.com> (raw)
In-Reply-To: <20190802151028.15590-5-rdapp@linux.ibm.com> (Robin Dapp's message of "Fri, 2 Aug 2019 17:10:23 +0200")
Robin Dapp <rdapp@linux.ibm.com> writes:
> This patch extends bb_ok_for_noce_convert_multiple_sets by a temporary
> cost estimation that can be used by noce_convert_multiple_sets.
I agree it looks like an omission that we didn't do this. The patch
looks OK to me (maybe independently of the rest?) bar minor things:
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index 253b8a96c1a..55205cac153 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -3333,11 +3333,13 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
> fewer than PARAM_MAX_RTL_IF_CONVERSION_INSNS sets. */
>
> static bool
> -bb_ok_for_noce_convert_multiple_sets (basic_block test_bb)
> +bb_ok_for_noce_convert_multiple_sets (basic_block test_bb, unsigned *cost)
The function comment should document COST.
> {
> rtx_insn *insn;
> unsigned count = 0;
> unsigned param = PARAM_VALUE (PARAM_MAX_RTL_IF_CONVERSION_INSNS);
> + bool speed_p = optimize_bb_for_speed_p (test_bb);
> + unsigned potential_cost = 0;
>
> FOR_BB_INSNS (test_bb, insn)
> {
> @@ -3373,9 +3375,14 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb)
> if (!can_conditionally_move_p (GET_MODE (dest)))
> return false;
>
> + rtx sset = single_set (insn);
This is already available as "set", unless I'm missing something.
> + potential_cost += pattern_cost (sset, speed_p);
> +
> count++;
> }
>
> + *cost += potential_cost;
> +
> /* If we would only put out one conditional move, the other strategies
> this pass tries are better optimized and will be more appropriate.
> Some targets want to strictly limit the number of conditional moves
> @@ -3414,11 +3421,15 @@ noce_process_if_block (struct noce_if_info *if_info)
> ??? For future expansion, further expand the "multiple X" rules. */
>
> /* First look for multiple SETS. */
> + unsigned int mcost = if_info->original_cost;
> + unsigned tmp_cost = if_info->original_cost;
Very minor, but it'd be good to be consistent about the choice
between unsigned and unsigned int. Maybe "old_cost" would be a
better name than "tmp_cost".
> if (!else_bb
> && HAVE_conditional_move
> && !HAVE_cc0
> - && bb_ok_for_noce_convert_multiple_sets (then_bb))
> + && bb_ok_for_noce_convert_multiple_sets (then_bb, &mcost))
> {
> + /* Temporarily set the original costs to what we estimated. */
> + if_info->original_cost = mcost;
> if (noce_convert_multiple_sets (if_info))
> {
> if (dump_file && if_info->transform)
> @@ -3427,6 +3438,8 @@ noce_process_if_block (struct noce_if_info *if_info)
> return TRUE;
> }
> }
> + /* Restore the original costs. */
> + if_info->original_cost = tmp_cost;
>
> bool speed_p = optimize_bb_for_speed_p (test_bb);
> unsigned int then_cost = 0, else_cost = 0;
I guess the save and restore only really need to be done inside the
outer "if". Not that the performance difference is going to be
noticeable, but maybe it would be a bit clearer to read.
Thanks,
Richard
next prev parent reply other threads:[~2019-08-06 20:14 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-02 15:10 [PATCH 0/9] Improve icvt "convert multiple" Robin Dapp
2019-08-02 15:10 ` [PATCH 5/9] ifcvt: Allow constants operands in noce_convert_multiple_sets Robin Dapp
2019-08-06 20:37 ` Richard Sandiford
2019-08-08 10:04 ` Robin Dapp
2019-10-27 14:12 ` Richard Sandiford
2019-08-02 15:10 ` [PATCH 3/9] ifcvt: Only created temporaries as needed Robin Dapp
2019-08-06 20:14 ` Richard Sandiford
2019-08-08 9:13 ` Robin Dapp
2019-08-02 15:10 ` [PATCH 1/9] ifcvt: Store the number of created cmovs Robin Dapp
2019-08-02 15:10 ` [PATCH 6/9] ifcvt: Extract cc comparison from jump Robin Dapp
2019-10-27 14:31 ` Richard Sandiford
2019-08-02 15:10 ` [PATCH 2/9] ifcvt: Use enum instead of transform_name string Robin Dapp
2019-08-06 21:03 ` Richard Sandiford
2019-08-08 22:18 ` Jeff Law
2019-08-02 15:10 ` [PATCH 8/9] ifcvt: Handle swap-style idioms differently Robin Dapp
2019-08-06 20:47 ` Richard Sandiford
2019-08-16 13:22 ` Robin Dapp
2019-08-16 15:38 ` Richard Sandiford
2019-08-17 23:48 ` Robin Dapp
2019-08-02 15:10 ` [PATCH 7/9] ifcvt: Emit two cmov variants and choose the less expensive one Robin Dapp
2019-10-27 14:38 ` Richard Sandiford
2019-08-02 15:10 ` [PATCH 4/9] ifcvt: Estimate original costs before convert_multiple Robin Dapp
2019-08-06 20:30 ` Richard Sandiford [this message]
2019-08-02 15:10 ` [PATCH 9/9] ifcvt: Also pass reversed cc comparison Robin Dapp
2019-10-27 15:27 ` Richard Sandiford
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=mptmugm6ogj.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=james.greenhalgh@arm.com \
--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).