From: Kito Cheng <kito.cheng@gmail.com>
To: Lehua Ding <lehua.ding@rivai.ai>
Cc: gcc-patches@gcc.gnu.org, rdapp.gcc@gmail.com, juzhe.zhong@rivai.ai
Subject: Re: [PATCH] RISC-V: Refactor and clean emit_{vlmax, nonvlmax}_xxx functions
Date: Thu, 31 Aug 2023 15:51:36 +0800 [thread overview]
Message-ID: <CA+yXCZDsHXvJMKvLfD-=RtcNSJjAXS5E+6t4oberVuqF5G7WTg@mail.gmail.com> (raw)
In-Reply-To: <20230830115327.1296036-1-lehua.ding@rivai.ai>
Thanks for the cleanup, the new interface is really much simpler than before!
Only few minor comment, you can go ahead to commit that after address
those comment.
(OK, I don't want to review whole patch again, it's really huge change :P
>
> - void set_rounding_mode (enum floating_point_rounding_mode mode)
> + void check_insn_flags ()
void check_insn_flags () const
> void emit_insn (enum insn_code icode, rtx *ops)
> {
> int opno = 0;
> + int num_ops = 0;
> /* It's true if any operand is memory operand. */
> bool any_mem_p = false;
> - /* It's true if all operands are mask operand. */
> - bool all_mask_p = true;
> - if (m_has_dest_p)
> +
> + /* Add dest operand. */
> + if (m_insn_flags & HAS_DEST_P)
> {
> any_mem_p |= MEM_P (ops[opno]);
> - all_mask_p &= GET_MODE_CLASS (GET_MODE (ops[opno])) == MODE_VECTOR_BOOL;
> add_output_operand (ops[opno++], m_dest_mode);
> + num_ops += 1;
Drop this
> }
>
> - if (m_fully_unmasked_p)
> + /* Add mask operand. */
> + if (m_insn_flags & USE_ONE_TRUE_MASK_P)
> + add_input_operand (gen_scalar_move_mask (m_mask_mode), m_mask_mode);
> + else if (m_insn_flags & USE_ALL_TRUES_MASK_P)
> add_all_one_mask_operand ();
> + else if (m_insn_flags & HAS_MASK_P)
> + {
> + machine_mode mode = insn_data[(int) icode].operand[m_opno].mode;
> + gcc_assert (mode != VOIDmode);
> + add_input_operand (ops[opno++], mode);
> + num_ops += 1;
> + }
Drop this
> - if (!m_use_real_merge_p)
> + /* Add merge operand. */
> + if (m_insn_flags & USE_VUNDEF_MERGE_P)
> add_vundef_operand ();
> + else if (m_insn_flags & HAS_MERGE_P)
> + {
> + machine_mode mode = insn_data[(int) icode].operand[m_opno].mode;
> + gcc_assert (mode != VOIDmode);
> + add_input_operand (ops[opno++], mode);
> + num_ops += 1;
> + }
> +
> + if (m_insn_flags & NULLARY_OP_P)
> + num_ops += 0;
> + else if (m_insn_flags & UNARY_OP_P)
> + num_ops += 1;
> + else if (m_insn_flags & BINARY_OP_P)
> + num_ops += 2;
> + else if (m_insn_flags & TERNARY_OP_P)
> + num_ops += 3;
num_ops = rather than += here.
> + else
> + gcc_unreachable ();
>
> - for (; opno < m_op_num; opno++)
> + /* Add the remain operands. */
> + for (; opno < num_ops; opno++)
for (;num_ops;num_ops--, opno++)
> {
> any_mem_p |= MEM_P (ops[opno]);
> - all_mask_p &= GET_MODE_CLASS (GET_MODE (ops[opno])) == MODE_VECTOR_BOOL;
> machine_mode mode = insn_data[(int) icode].operand[m_opno].mode;
> /* 'create_input_operand doesn't allow VOIDmode.
> According to vector.md, we may have some patterns that do not have
> @@ -194,46 +251,51 @@ public:
> add_input_operand (ops[opno], mode);
> }
>
> - if (m_needs_avl_p)
> + /* Add vl operand. */
> + rtx len = m_vl_op;
> + machine_mode mode = VECTOR_MODE_P (m_dest_mode) ? m_dest_mode : m_mask_mode;
> + if (m_vlmax_p)
> {
> - rtx len = m_vl_op;
> - machine_mode mode
> - = VECTOR_MODE_P (m_dest_mode) ? m_dest_mode : m_mask_mode;
> - if (m_vlmax_p)
> + if (riscv_v_ext_vls_mode_p (mode))
> + {
> + /* VLS modes always set VSETVL by
> + "vsetvl zero, rs1/imm". */
> + poly_uint64 nunits = GET_MODE_NUNITS (mode);
> + len = gen_int_mode (nunits, Pmode);
> + if (!satisfies_constraint_K (len))
> + len = force_reg (Pmode, len);
> + m_vlmax_p = false;
> + }
> + else if (const_vlmax_p (mode))
> + {
> + /* Optimize VLS-VLMAX code gen, we can use vsetivli instead of
> + the vsetvli to obtain the value of vlmax. */
> + poly_uint64 nunits = GET_MODE_NUNITS (mode);
> + len = gen_int_mode (nunits, Pmode);
> + m_vlmax_p = false;
> + }
> + else if (can_create_pseudo_p ())
> {
> - if (riscv_v_ext_vls_mode_p (mode))
> - {
> - /* VLS modes always set VSETVL by
> - "vsetvl zero, rs1/imm". */
> - poly_uint64 nunits = GET_MODE_NUNITS (mode);
> - len = gen_int_mode (nunits, Pmode);
> - if (!satisfies_constraint_K (len))
> - len = force_reg (Pmode, len);
> - m_vlmax_p = false; /* It has became NONVLMAX now. */
> - }
> - else if (const_vlmax_p (mode))
> - {
> - /* Optimize VLS-VLMAX code gen, we can use vsetivli instead of
> - the vsetvli to obtain the value of vlmax. */
> - poly_uint64 nunits = GET_MODE_NUNITS (mode);
> - len = gen_int_mode (nunits, Pmode);
> - m_vlmax_p = false; /* It has became NONVLMAX now. */
> - }
> - else if (can_create_pseudo_p ())
> - {
> - len = gen_reg_rtx (Pmode);
> - emit_vlmax_vsetvl (mode, len);
> - }
> + len = gen_reg_rtx (Pmode);
> + emit_vlmax_vsetvl (mode, len);
> }
> - add_input_operand (len, Pmode);
> + else
> + gcc_assert (len != NULL_RTX);
I don't see the gcc_assert here before, and seems like that should
move to another place
> }
> + else
> + /* Must specify vl when not vlmax. */
> + gcc_assert (len != NULL_RTX);
Like here and drop the else since we need this assert no matter
m_vlmax_p true or false
diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index 3e7caf48742..64e22177e81 100644
--- a/gcc/config/riscv/riscv-v.cc
+++ b/gcc/config/riscv/riscv-v.cc
@@ -279,12 +279,8 @@ public:
len = gen_reg_rtx (Pmode);
emit_vlmax_vsetvl (mode, len);
}
- else
- gcc_assert (len != NULL_RTX);
}
- else
- /* Must specify vl when not vlmax. */
- gcc_assert (len != NULL_RTX);
+ gcc_assert (len != NULL_RTX);
add_input_operand (len, Pmode);
/* Add tail and mask policy operands. */
> @@ -1212,7 +834,7 @@ emit_vlmax_masked_gather_mu_insn (rtx target, rtx op, rtx sel, rtx mask)
> p q r s t u v w # v11 destination register
> e d c b a # v1 source vector
> 1 0 0 1 1 1 0 1 # v0 mask vector
> -
> +make
Ha, this must be a mistake, right? :P
next prev parent reply other threads:[~2023-08-31 7:51 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-30 11:53 [PATCH] RISC-V: Refactor and clean emit_{vlmax,nonvlmax}_xxx functions Lehua Ding
2023-08-31 7:51 ` Kito Cheng [this message]
2023-08-31 8:59 ` [PATCH] RISC-V: Refactor and clean emit_{vlmax, nonvlmax}_xxx functions Lehua Ding
2023-08-31 9:16 ` [PATCH] RISC-V: Refactor and clean emit_{vlmax,nonvlmax}_xxx functions Robin Dapp
2023-08-31 9:59 ` Lehua Ding
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='CA+yXCZDsHXvJMKvLfD-=RtcNSJjAXS5E+6t4oberVuqF5G7WTg@mail.gmail.com' \
--to=kito.cheng@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=juzhe.zhong@rivai.ai \
--cc=lehua.ding@rivai.ai \
--cc=rdapp.gcc@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).