From: "Fei Gao" <gaofei@eswincomputing.com>
To: jeffreyalaw <jeffreyalaw@gmail.com>,
gcc-patches <gcc-patches@gcc.gnu.org>
Cc: "Kito Cheng" <kito.cheng@gmail.com>,
"Palmer Dabbelt" <palmer@dabbelt.com>
Subject: Re: Re: [PATCH 2/4] [ifcvt] if convert x=c ? y+z : y by RISC-V Zicond like insns
Date: Tue, 28 Nov 2023 10:57:58 +0800 [thread overview]
Message-ID: <20231128105758661692145@eswincomputing.com> (raw)
In-Reply-To: <805ad185-bb70-485c-a806-df5d42455dfb@gmail.com>
On 2023-11-20 14:59 Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
>On 10/30/23 01:25, Fei Gao wrote:
>> Conditional add, if zero
>> rd = (rc == 0) ? (rs1 + rs2) : rs1
>> -->
>> czero.nez rd, rs2, rc
>> add rd, rs1, rd
>>
>> Conditional add, if non-zero
>> rd = (rc != 0) ? (rs1 + rs2) : rs1
>> -->
>> czero.eqz rd, rs2, rc
>> add rd, rs1, rd
>>
>> Co-authored-by: Xiao Zeng<zengxiao@eswincomputing.com>
>>
>> gcc/ChangeLog:
>>
>> * ifcvt.cc (noce_emit_czero): helper for noce_try_cond_zero_arith
>> (noce_try_cond_zero_arith): handler for condtional zero op
>> (noce_process_if_block): add noce_try_cond_zero_arith with hook control
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.target/riscv/zicond_ifcvt_opt.c: New test.
>> ---
>> gcc/ifcvt.cc | 112 +++++++++++++++
>> .../gcc.target/riscv/zicond_ifcvt_opt.c | 130 ++++++++++++++++++
>> 2 files changed, 242 insertions(+)
>> create mode 100644 gcc/testsuite/gcc.target/riscv/zicond_ifcvt_opt.c
>>
>> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
>> index a0af553b9ff..4f98c1c7bf9 100644
>> --- a/gcc/ifcvt.cc
>> +++ b/gcc/ifcvt.cc
>> @@ -781,12 +781,14 @@ static bool noce_try_store_flag_constants (struct noce_if_info *);
>> static bool noce_try_store_flag_mask (struct noce_if_info *);
>> static rtx noce_emit_cmove (struct noce_if_info *, rtx, enum rtx_code, rtx,
>> rtx, rtx, rtx, rtx = NULL, rtx = NULL);
>> +static rtx noce_emit_czero (struct noce_if_info *, enum rtx_code, rtx, rtx);
>> static bool noce_try_cmove (struct noce_if_info *);
>> static bool noce_try_cmove_arith (struct noce_if_info *);
>> static rtx noce_get_alt_condition (struct noce_if_info *, rtx, rtx_insn **);
>> static bool noce_try_minmax (struct noce_if_info *);
>> static bool noce_try_abs (struct noce_if_info *);
>> static bool noce_try_sign_mask (struct noce_if_info *);
>> +static bool noce_try_cond_zero_arith (struct noce_if_info *);
>>
>> /* Return the comparison code for reversed condition for IF_INFO,
>> or UNKNOWN if reversing the condition is not possible. */
>> @@ -1831,6 +1833,32 @@ noce_emit_cmove (struct noce_if_info *if_info, rtx x, enum rtx_code code,
>> return NULL_RTX;
>> }
>>
>> +static rtx
>> +noce_emit_czero (struct noce_if_info *if_info, enum rtx_code czero_code, rtx z, rtx target)
>Every function needs a comment describing what the function does, it's
>return value(s) and its arguments. There are many examples in ifcvt.cc
>you can use to guide you. I might start with something like this:
>
>/* Emit a conditional zero, returning the location of the result
> or NULL_RTX upon failure.
>
> IF_INFO describes the if-conversion scenario under consideration.
> CZERO_CODE selects the condition (EQ/NE).
> Z is the nonzero operand of the conditional move
> TARGET is the desired output register. */
>
>Or something like that. I would suggest renaming "Z" to something more
>meaningful.
Hi Jeff
Thanks for your patients. All comments regarding coding style have been addressed in new patches.
>
>
>
>>
>> +/* Convert x = c ? y + z : y or x = c ? y : y + z. */
>> +
>> +static bool
>> +noce_try_cond_zero_arith (struct noce_if_info *if_info)
>The function comment really should be improved. For example it doesn't
>indicate what the return value is.
>
>> +
>> + /* cond must be EQ or NEQ comparision of a reg and 0. */
>In general when you refer to a variable in a comment, do so in upper
>case. Use NE rather than NEQ as the former is how most code refers to a
>not-equal rtx code.
>
>
>> + if (GET_CODE (cond) != NE && GET_CODE (cond) != EQ)
>> + return false;
>> + if (!REG_P (XEXP (cond, 0)) || !rtx_equal_p (XEXP (cond, 1), const0_rtx))
>> + return false;
>> +
>> + /* check y + z:y*/
>> + if (GET_CODE (a) == PLUS && REG_P (XEXP (a, 0)) && REG_P (XEXP (a, 1))
>> + && REG_P (b) && rtx_equal_p (XEXP (a, 0), b))
>Write comments as complete sentences.
>
>> + {
>> + common = b;
>> + z = XEXP (a, 1);
>Rather than "z" use a more descriptive variable name.
>
>
>> +
>> + /* If we have x = c ? x + z : x, use a new reg to avoid modifying x */
>> + if (common && rtx_equal_p (common, if_info->x))
>> + target = gen_reg_rtx (mode);
>> + else
>> + target = if_info->x;
>if-conversion runs both before and after register allocation. So you
>have to handle the case where you can not generate new registers. Use
>can_create_pseudo_p () to test for that. You may need to fail if you
>can't generate a new register.
1. In find_if_header function, I found the following piece of codes:
if (!reload_completed && noce_find_if_block(...)), and find_if_header must
be called before noce_try_cond_zero_arith().
2. In noce_try_strore_flag_constants, new registers are also generated
without can_create_pseudo_p() check.
So I guess no need to add can_create_pseudo_p() here.
>
>> +
>> + target = noce_emit_czero (if_info, czero_code, z, target);
>> + if (!target)
>> + {
>> + end_sequence ();
>> + return false;
>> + }
>> +
>> + target = expand_simple_binop (mode, PLUS, common, target, if_info->x, 0,
>> + OPTAB_DIRECT);
>I think you want OPTAB_WIDEN and in the other calls.
>
>> @@ -3975,6 +4085,8 @@ noce_process_if_block (struct noce_if_info *if_info)
>> goto success;
>> if (noce_try_store_flag_mask (if_info))
>> goto success;
>> + if (targetm.have_cond_zero () && noce_try_cond_zero_arith (if_info))
>> + goto success;
>Replace targetm.have_cond_zero with HAVE_conditional_move since that's
>the RTL primitive we're building from.
Done.
>
>
>>
>> +**test_ADD_ceqz:
>> +** czero\.eqz a3,a2,a3
>> +** add a0,a1,a3
>> +** ret
>Please don't use explicit registers unless you know they will always be
>correct. In this sequence there's no guarantee the register allocator
>will put the result of the czero.eqz into $a3. Use a suitable regexp
>instead to match a variety of registers. This will be an issue for all
>your new tests.
Done.
BR,
Fei
>
>
>
>Jeff
next prev parent reply other threads:[~2023-11-28 2:58 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-30 7:25 [PATCH 0/4] add support for conditional zero operation Fei Gao
2023-10-30 7:25 ` [PATCH 1/4] [RISC-V]add hook to control Zicond based ifcvt opt Fei Gao
2023-10-30 15:12 ` Jeff Law
2023-10-30 7:25 ` [PATCH 2/4] [ifcvt] if convert x=c ? y+z : y by RISC-V Zicond like insns Fei Gao
2023-10-30 16:36 ` Jeff Law
2023-10-31 2:53 ` Fei Gao
2023-10-30 18:41 ` Jeff Law
2023-10-30 19:16 ` Jeff Law
2023-10-31 3:35 ` Fei Gao
2023-11-20 6:46 ` Jeff Law
2023-11-28 2:46 ` Fei Gao
2023-11-28 5:05 ` Jeff Law
2023-11-20 6:59 ` Jeff Law
2023-11-28 2:57 ` Fei Gao [this message]
2023-11-29 4:46 ` Jeff Law
2023-10-30 7:25 ` [PATCH 3/4] [ifcvt] if convert x=c ? y op z " Fei Gao
2023-11-20 7:02 ` Jeff Law
2023-10-30 7:25 ` [PATCH 4/4] [ifcvt] if convert x=c ? y&z " Fei Gao
2023-10-30 18:46 ` Jeff Law
2023-11-20 7:10 ` Jeff Law
2023-11-28 3:04 ` Fei Gao
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=20231128105758661692145@eswincomputing.com \
--to=gaofei@eswincomputing.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jeffreyalaw@gmail.com \
--cc=kito.cheng@gmail.com \
--cc=palmer@dabbelt.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).