From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk1-xa36.google.com (mail-vk1-xa36.google.com [IPv6:2607:f8b0:4864:20::a36]) by sourceware.org (Postfix) with ESMTPS id E75453858C2B for ; Thu, 31 Aug 2023 07:51:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E75453858C2B Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-vk1-xa36.google.com with SMTP id 71dfb90a1353d-48d0ceedda1so215556e0c.3 for ; Thu, 31 Aug 2023 00:51:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1693468308; x=1694073108; darn=gcc.gnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=obFqixCpbuBPkWW6PfxtdEuivwoafc8i5zsXOREXXc8=; b=OEZqhaAYEQG0ZBvnDGb8Pv549UqdzCTjipjH7RX29UGGXW/hYOLBRhKqRzZJaC/Btu atb+1uEPy2tsVGxyaHQGJ/1np9YzLKEMUfChvBVIxpErNvkAE0X8IDqx2DEfBaYCgxMo TkQol660GPBO76N2Gjo9vheeOb7UDXsYIwiZC5yyzLQ+vx5wUP15bsnuNGmJyG6RLmVu ZsBSRDTgrmCy9k8zqLLnF5O33n49Uceg9AaeGNQ0RVG4mhIsR6Z1CZyM8CoyOHW5s9VG Yj4zKdK8S0q6H9wzUrXfZ8IlrVGkGS09GhMf4wN0DdjvzhSn8Xdvm8hNjkXR8P4F0bAc YybQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693468308; x=1694073108; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=obFqixCpbuBPkWW6PfxtdEuivwoafc8i5zsXOREXXc8=; b=jfwlJIB53GDRK+N/ylUcm87CpGLlS+t2EYcUZ8+6A0TzZ6X0tzbDBjeV680YdEvNtw j33QUgnb3c772H6e57G+gmSdgTDH+/9r4Aw4TZM+nPfc7MXqFUIWoQNSYAKVTfOYRWdH DqtwMwGdWfpmFXX/v9lK2CD8rT9RwteTmPjDvhfd4YMCheAcu3TUu+C+VMqEDbKmFyQr oyIqqEL1/vZKZI9lQYbRPBTuNEF1b0RUE7TAm/ZB8Hmt2w5G9I1u7ZbfC20O4EG+gj3H 5cRQD4YILDmWRdwULnD4adLe2b+C0grE6/nYwRJKVq6YZFjlzGWdbMwFVy3g3LccyKvv sCiw== X-Gm-Message-State: AOJu0Yz7aT6n/8rCrSxKVYMAwbMAqaNmYabMqFBeilT4Fdlq9QtGMw9G 0CXBQ0ROXA9xun1JmsyGmu06Fuu6ldOBKFdf/fQ= X-Google-Smtp-Source: AGHT+IFfmA9HWtxWKTgB5ajulo5/Akf38ZMPTEMafcEC4Qs1Cnaygg/dZfI6/lk6GmL1WFwbJ6V3GWMFGJO8fUBNiLI= X-Received: by 2002:a67:ffc5:0:b0:44d:4d7f:bcc4 with SMTP id w5-20020a67ffc5000000b0044d4d7fbcc4mr4492344vsq.5.1693468307981; Thu, 31 Aug 2023 00:51:47 -0700 (PDT) MIME-Version: 1.0 References: <20230830115327.1296036-1-lehua.ding@rivai.ai> In-Reply-To: <20230830115327.1296036-1-lehua.ding@rivai.ai> From: Kito Cheng Date: Thu, 31 Aug 2023 15:51:36 +0800 Message-ID: Subject: Re: [PATCH] RISC-V: Refactor and clean emit_{vlmax, nonvlmax}_xxx functions To: Lehua Ding Cc: gcc-patches@gcc.gnu.org, rdapp.gcc@gmail.com, juzhe.zhong@rivai.ai Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,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: 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