From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpbg150.qq.com (smtpbg150.qq.com [18.132.163.193]) by sourceware.org (Postfix) with ESMTPS id F094B3858D37 for ; Thu, 31 Aug 2023 08:59:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F094B3858D37 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivai.ai Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivai.ai X-QQ-mid: bizesmtp65t1693472359t6m8w437 Received: from [10.101.11.9] ( [113.104.210.73]) by bizesmtp.qq.com (ESMTP) with id ; Thu, 31 Aug 2023 16:59:18 +0800 (CST) X-QQ-SSF: 01400000000000C0F000000A0000000 X-QQ-FEAT: Z2qCHCq1/923f8lT6+sNGLy1VFDI87Dpj8EvEZoCTdm64KiC1M+m/9a1Dzk5m j34DGEXvJLVi+cwqzsmPH2qUH2ui559yh4K4aSroTokVZkSnhONCLnf4nXhntM6Eebhlmk+ 6Ks05joNaaWWsl0rbhI7WqYyImAq9EyxDjiP86QC/J+uO7pL0WYhwPJJsX0M7F/8GCmWJ9h Pa53bT9tfcJmG0QWdlqtgzCbI3a4eYAipjanTRPFeRBzFwSE9g4EagYNJLcGbgYl3TNkZcq 0wjmFsedIuerGsIHtbZ5oTkgJjkNFRzG8Lz3BnXp3sV3CnaRpJvL213XhoumaZ0aYoc1xXS 7m/SXvrVpmWk2SpHD+gMlbTBetKCFadxJsGj5rJvScmqnQQdByq/RDhzwK92bFCc1pG/DWV X-QQ-GoodBg: 2 X-BIZMAIL-ID: 16483957902726562519 Message-ID: Date: Thu, 31 Aug 2023 16:59:18 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] RISC-V: Refactor and clean emit_{vlmax, nonvlmax}_xxx functions Content-Language: en-US To: Kito Cheng Cc: gcc-patches@gcc.gnu.org, rdapp.gcc@gmail.com, juzhe.zhong@rivai.ai References: <20230830115327.1296036-1-lehua.ding@rivai.ai> From: Lehua Ding In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:rivai.ai:qybglogicsvrgz:qybglogicsvrgz6a-0 X-Spam-Status: No, score=-9.1 required=5.0 tests=BAYES_00,FORGED_MUA_MOZILLA,GIT_PATCH_0,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Committed after addressing these comments. Many thanks to Kito for taking the time to review such a large patch :) On 2023/8/31 15:51, Kito Cheng wrote: > 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 > -- Best, Lehua