From: Richard Sandiford <richard.sandiford@arm.com>
To: Hongyu Wang <wwwhhhyyy333@gmail.com>
Cc: gcc-patches@gcc.gnu.org, ubizjak@gmail.com, hongtao.liu@intel.com
Subject: Re: [PATCH 2/3] [APX CCMP] Adjust startegy for selecting ccmp candidates
Date: Wed, 05 Jun 2024 10:21:50 +0100 [thread overview]
Message-ID: <mptfrtrvj69.fsf@arm.com> (raw)
In-Reply-To: <CA+OydW=mVcjn3iYJ6PWwYipPJs100b25rmMbKLREs+hQtb869g@mail.gmail.com> (Hongyu Wang's message of "Wed, 15 May 2024 16:25:27 +0800")
Hongyu Wang <wwwhhhyyy333@gmail.com> writes:
> CC'd Richard for ccmp part as previously it is added only for aarch64.
> The original logic will not interrupted since if
> aarch64_gen_ccmp_first succeeded, aarch64_gen_ccmp_next will also
> success, the cmp/fcmp and ccmp/fccmp supports all GPI/GPF, and the
> prepare_operand will fixup the input that cmp supports but ccmp not,
> so ret/ret2 will all be valid when comparing cost.
> Thanks in advance.
Sorry for the slow review.
> Hongyu Wang <hongyu.wang@intel.com> 于2024年5月15日周三 16:22写道:
>>
>> For general ccmp scenario, the tree sequence is like
>>
>> _1 = (a < b)
>> _2 = (c < d)
>> _3 = _1 & _2
>>
>> current ccmp expanding will try to swap compare order for _1 and _2,
>> compare the cost/cost2 between compare _1 and _2 first, then return the
>> sequence with lower cost.
>>
>> For x86 ccmp, we don't support FP compare as ccmp operand, but we
>> support fp comi + int ccmp sequence. With current cost comparison
>> model, the fp comi + int ccmp can never be generated since it doesn't
>> check whether expand_ccmp_next returns available result and the rtl
>> cost for the empty ccmp sequence is always smaller.
>>
>> Check the expand_ccmp_next result ret and ret2, returns the valid one
>> before cost comparison.
>>
>> gcc/ChangeLog:
>>
>> * ccmp.cc (expand_ccmp_expr_1): Check ret and ret2 of
>> expand_ccmp_next, returns the valid one first before
>> comparing cost.
>> ---
>> gcc/ccmp.cc | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/ccmp.cc b/gcc/ccmp.cc
>> index 7cb525addf4..4b424220068 100644
>> --- a/gcc/ccmp.cc
>> +++ b/gcc/ccmp.cc
>> @@ -247,7 +247,17 @@ expand_ccmp_expr_1 (gimple *g, rtx_insn **prep_seq, rtx_insn **gen_seq)
>> cost2 = seq_cost (prep_seq_2, speed_p);
>> cost2 += seq_cost (gen_seq_2, speed_p);
>> }
>> - if (cost2 < cost1)
>> +
>> + /* For x86 target the ccmp does not support fp operands, but
>> + have fcomi insn that can produce eflags and then do int
>> + ccmp. So if one of the op is fp compare, ret1 or ret2 can
>> + fail, and the cost of the corresponding empty seq will
>> + always be smaller, then the NULL sequence will be returned.
>> + Add check for ret and ret2, returns the available one if
>> + the other is NULL. */
I think the more fundamental point is that the cost of a failed
expansion isn't meaningful. So how about:
/* It's possible that one expansion succeeds and the other fails.
For example, x86 has int ccmp but not fp ccmp, and so a combined
fp and int comparison must be ordered such that the fp comparison
happens first. The costs are not meaningful for failed
expansions. */
>> + if ((!ret && ret2)
>> + || (!(ret && !ret2)
>> + && cost2 < cost1))
I think this simplifies to:
if (ret2 && (!ret1 || cost2 < cost1))
OK with those changes, thanks.
Richard
>> {
>> *prep_seq = prep_seq_2;
>> *gen_seq = gen_seq_2;
>> --
>> 2.31.1
>>
next prev parent reply other threads:[~2024-06-05 9:21 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-15 8:20 [PATCH 0/3] Support Intel APX CCMP Hongyu Wang
2024-05-15 8:20 ` [PATCH 1/3] [APX CCMP] Support " Hongyu Wang
2024-05-31 4:47 ` Hongtao Liu
2024-05-15 8:20 ` [PATCH 2/3] [APX CCMP] Adjust startegy for selecting ccmp candidates Hongyu Wang
2024-05-15 8:25 ` Hongyu Wang
2024-05-23 8:27 ` Hongyu Wang
2024-05-30 3:11 ` Hongyu Wang
2024-06-05 9:21 ` Richard Sandiford [this message]
2024-06-06 7:13 ` Hongyu Wang
2024-05-15 8:20 ` [PATCH 3/3] [APX CCMP] Support ccmp for float compare Hongyu Wang
2024-05-31 4:48 ` Hongtao Liu
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=mptfrtrvj69.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hongtao.liu@intel.com \
--cc=ubizjak@gmail.com \
--cc=wwwhhhyyy333@gmail.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).