From: "Xiao Zeng" <zengxiao@eswincomputing.com>
To: jeffreyalaw <jeffreyalaw@gmail.com>,
gcc-patches <gcc-patches@gcc.gnu.org>
Cc: research_trasio <research_trasio@irq.a4lg.com>,
kito.cheng <kito.cheng@gmail.com>,
zhengyu <zhengyu@eswincomputing.com>,
eri-sw-toolchain <eri-sw-toolchain@eswincomputing.com>
Subject: Re: Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0
Date: Sat, 29 Jul 2023 17:14:30 +0800 [thread overview]
Message-ID: <2023072917142990536214@eswincomputing.com> (raw)
In-Reply-To: <51e4b6e3-ef07-8462-f2d0-fc56f5299391@gmail.com>
On Sat, Jul 29, 2023 at 04:59:00 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
>On 7/19/23 04:11, Xiao Zeng wrote:
>
>> + else if (TARGET_ZICOND
>> + && (code == EQ || code == NE)
>> + && GET_MODE_CLASS (mode) == MODE_INT)
>> + {
>> + need_eq_ne_p = true;
>> + /* 0 + imm */
>> + if (GET_CODE (cons) == CONST_INT && cons == const0_rtx
>> + && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
>> + {
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + alt = force_reg (mode, alt);
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + cons, alt)));
>> + return true;
>> + }
>> + /* imm + imm */
>> + else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
>> + && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
>> + {
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + alt = force_reg (mode, alt);
>> + rtx temp1 = gen_reg_rtx (mode);
>> + rtx temp2 = GEN_INT(-1 * INTVAL (cons));
>> + riscv_emit_binary(PLUS, temp1, alt, temp2);
>So in this sequence you're just computing a constant since both ALT and
>CONS are constants. It's better to just form the constant directly,
>then force that into a register because it'll make the costing more
>correct, particularly if the resulting constant needs more than one
>instruction to synthesize.
Fixed
>
>And a nit. There should always be a space between a function name and
>its argument list.
Fixed
>
>
>
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + const0_rtx, alt)));
>> + riscv_emit_binary(PLUS, dest, dest, cons);
>> + return true;
>I don't see how this can be correct from a code generation standpoint.
>You compute ALT-CONS into TEMP1 earlier. But you never use TEMP1 after
>that. I think you meant to use TEMP1 instead of ALT as the false arm if
>the IF-THEN-ELSE you constructed.
Fixed
>
>In general you should be using CONST0_RTX (mode) rather than const0_rtx.
>
Fixed
>> + }
>> + /* imm + reg */
>> + else if (GET_CODE (cons) == CONST_INT && cons != const0_rtx
>> + && GET_CODE (alt) == REG)
>> + {
>> + /* Optimize for register value of 0. */
>> + if (op0 == alt && op1 == const0_rtx)
>> + {
>> + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + cons = force_reg (mode, cons);
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + cons, alt)));
>> + return true;
>> + }
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + rtx temp1 = gen_reg_rtx (mode);
>> + rtx temp2 = GEN_INT(-1 * INTVAL (cons));
>> + riscv_emit_binary(PLUS, temp1, alt, temp2);
>Here you have to be careful if CONS is -2048. You negate it resulting
>in +2048 which can't be used in an addi. This will cause the entire
>sequence to fail due to an unrecognized insn. It would be better to
>handle that scenario directly so the generated sequence is still valid.
>
>By generating recognizable code in that case we let the costing model
>determine if the conditional move sequence is better than the branching
>sequence.
Thank you for pointing out this special situation, it has been fixed
>
>
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + const0_rtx, alt)));
>I think we have the same problem with the use of ALT here rather than
>TEMP1 that we had in the previous case.
Fixed
>
>
>
>> + /* reg + imm */
>> + else if (GET_CODE (cons) == REG
>> + && GET_CODE (alt) == CONST_INT && alt != const0_rtx)
>> + {
>> + /* Optimize for register value of 0. */
>> + if (op0 == cons && op1 == const0_rtx)
>> + {
>> + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + alt = force_reg (mode, alt);
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + cons, alt)));
>> + return true;
>> + }
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + rtx cond = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + rtx temp1 = gen_reg_rtx (mode);
>> + rtx temp2 = GEN_INT(-1 * INTVAL (alt));
>> + riscv_emit_binary(PLUS, temp1, cons, temp2);
>> + emit_insn (gen_rtx_SET (dest,
>> + gen_rtx_IF_THEN_ELSE (mode, cond,
>> + temp1, const0_rtx)));
>> + riscv_emit_binary(PLUS, dest, dest, alt);
>> + return true;
>This has basically the same issues as the imm + reg case.
Fixed
>
>
>> + }
>> + /* reg + reg */
>> + else if (GET_CODE (cons) == REG && GET_CODE (alt) == REG)
>> + {
>> + rtx reg1 = gen_reg_rtx (mode);
>> + rtx reg2 = gen_reg_rtx (mode);
>> + riscv_emit_int_compare (&code, &op0, &op1, need_eq_ne_p);
>> + rtx cond1 = gen_rtx_fmt_ee (code, GET_MODE (op0), op0, op1);
>> + rtx cond2 = gen_rtx_fmt_ee (code == NE ? EQ : NE,
>> + GET_MODE (op0), op0, op1);
>> + emit_insn (gen_rtx_SET (reg2,
>> + gen_rtx_IF_THEN_ELSE (mode, cond2,
>> + const0_rtx, cons)));
>> + emit_insn (gen_rtx_SET (reg1,
>> + gen_rtx_IF_THEN_ELSE (mode, cond1,
>> + const0_rtx, alt)));
>> + riscv_emit_binary(IOR, dest, reg1, reg2);
>> + return true;
>This probably should detect the case where alt or cons is the same as
>op0. Otherwise I would expect the costing model to reject some cases
>that are actually profitable.
>
Fixed
>
>> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
>> index 6b8c2e8e268..b4147c7a79c 100644
>> --- a/gcc/config/riscv/riscv.md
>> +++ b/gcc/config/riscv/riscv.md
>> @@ -2484,7 +2484,7 @@
>> (if_then_else:GPR (match_operand 1 "comparison_operator")
>> (match_operand:GPR 2 "reg_or_0_operand")
>> (match_operand:GPR 3 "sfb_alu_operand")))
>> - "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV"
>> + "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV || TARGET_ZICOND"
>Note the predicate for operand2 will reject all constants except 0.
>
>Anyway, I'm still cleaning this up and adding the bits from Ventana
>which handle the relational conditions as well as FP conditionals.
>
>Jeff
1 Thank you for Jeff's code review comments. I have made the modifications
and submitted the V2-patch[3/5].
2 For the calculation method of cost, I hope to submit a separate patch[cost]
after the V2-patch[3/5] merged into master, which will focus on explaining
the reasons for calculating cost in the same way as in patch[4/5].
3 At the same time, I realized that for Zicond's series of patches, it would be
better to split them into separate patches and submit them to the community
code review. Therefore, I plan to submit:
V2-patch[3/5]
patch[cost]
V2-patch[4/5]
V2-patch[5/5]
I will only submit subsequent patches after the previous patch enters the master.
4. In V2-patch[3/5], Zicond's cost calculation is not involved, therefore, all test
cases are skipped with "- O0" and "- Os". I will remove the "- Os" constraint from
the test case in patch[cost].
5 The address for V2-patch[3/5] is: https://gcc.gnu.org/pipermail/gcc-patches/2023-July/625781.html
Thanks
Xiao Zeng
next prev parent reply other threads:[~2023-07-29 9:14 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-19 10:11 [PATCH 0/5] Recognize Zicond extension Xiao Zeng
2023-07-19 10:11 ` [PATCH 1/5] [RISC-V] " Xiao Zeng
2023-07-25 16:35 ` Jeff Law
2023-07-26 21:11 ` Jeff Law
2023-07-19 10:11 ` [PATCH 2/5] [RISC-V] Generate Zicond instruction for basic semantics Xiao Zeng
2023-07-25 16:35 ` Jeff Law
2023-07-26 17:53 ` Jeff Law
2023-08-01 11:18 ` Richard Sandiford
2023-08-02 6:22 ` Jeff Law
2023-08-02 10:05 ` Richard Sandiford
2023-08-02 16:56 ` Jeff Law
2023-07-26 21:14 ` Jeff Law
2023-07-19 10:11 ` [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0 Xiao Zeng
2023-07-25 17:32 ` Jeff Law
2023-07-25 17:55 ` Andreas Schwab
2023-07-27 5:44 ` Xiao Zeng
2023-07-28 15:09 ` Jeff Law
2023-07-29 9:48 ` Xiao Zeng
2023-07-28 20:59 ` Jeff Law
2023-07-29 9:14 ` Xiao Zeng [this message]
2023-08-03 4:59 ` Jeff Law
2023-08-02 6:34 ` Jeff Law
2023-07-19 10:11 ` [PATCH 4/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to non-zero Xiao Zeng
2023-08-07 17:36 ` Jeff Law
2023-07-19 10:11 ` [PATCH 5/5] [RISC-V] Generate Zicond instruction for conditional execution Xiao Zeng
2023-07-25 17:51 ` [PATCH 0/5] Recognize Zicond extension Jeff Law
2023-07-27 8:43 ` Xiao Zeng
2023-07-27 14:43 ` Jeff Law
2023-07-28 6:34 ` Xiao Zeng
2023-07-28 15:03 ` Jeff Law
2023-07-29 10:01 ` Xiao Zeng
2023-08-03 2:59 ` Jeff Law
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=2023072917142990536214@eswincomputing.com \
--to=zengxiao@eswincomputing.com \
--cc=eri-sw-toolchain@eswincomputing.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jeffreyalaw@gmail.com \
--cc=kito.cheng@gmail.com \
--cc=research_trasio@irq.a4lg.com \
--cc=zhengyu@eswincomputing.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).