| Message ID | 20231220122548.396-1-cooper.joshua@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Support XTheadVector extension | expand |
On 12/20/23 05:25, Jun Sha (Joshua) wrote: > This patch moves the definition of the enums lst_type and > frm_op_type into riscv-vector-builtins-bases.h and removes > the static visibility of fold_fault_load(), so these > can be used in other compile units. > > gcc/ChangeLog: > > * config/riscv/riscv-vector-builtins-bases.cc (enum lst_type): > (enum frm_op_type): move to riscv-vector-builtins-bases.h > * config/riscv/riscv-vector-builtins-bases.h > (GCC_RISCV_VECTOR_BUILTINS_BASES_H): Add header files. > (enum lst_type): move from > (enum frm_op_type): riscv-vector-builtins-bases.cc > (fold_fault_load): riscv-vector-builtins-bases.cc I'm largely hoping to leave the heavy review lifting here to Juzhe who knows GCC's RV vector bits as well as anyone. Just one small issue. Would it be better to prototype fold_fault_load elsewhere and avoid the gimple.h inclusion in riscv-vector-builtins-bases.h? Perhaps riscv-protos.h? You might consider prefixing the function name with riscv_. It's not strictly necessary, but it appears to be relatively common in risc-v port. Thanks, Jeff
Hi Jeff, Perhaps fold_fault_load cannot be moved to riscv-protos.h since gimple_folder is declared in riscv-vector-builtins.h. It's not reasonable to include riscv-vector-builtins.h in riscv-protos.h. In fact, fold_fault_load is defined specially for some builtin functions, and it would be better to just prototype in riscv-vector-builtins-bases.h. Joshua ------------------------------------------------------------------ 发件人：Jeff Law <jeffreyalaw@gmail.com> 发送时间：2023年12月21日(星期四) 02:14 收件人："Jun Sha (Joshua)"<cooper.joshua@linux.alibaba.com>; "gcc-patches"<gcc-patches@gcc.gnu.org> 抄 送："jim.wilson.gcc"<jim.wilson.gcc@gmail.com>; palmer<palmer@dabbelt.com>; andrew<andrew@sifive.com>; "philipp.tomsich"<philipp.tomsich@vrull.eu>; "christoph.muellner"<christoph.muellner@vrull.eu>; "juzhe.zhong"<juzhe.zhong@rivai.ai>; Jin Ma<jinma@linux.alibaba.com>; Xianmiao Qu<cooper.qu@linux.alibaba.com> 主 题：Re: [PATCH v3 1/6] RISC-V: Refactor riscv-vector-builtins-bases.cc On 12/20/23 05:25, Jun Sha (Joshua) wrote: > This patch moves the definition of the enums lst_type and > frm_op_type into riscv-vector-builtins-bases.h and removes > the static visibility of fold_fault_load(), so these > can be used in other compile units. > > gcc/ChangeLog: > > * config/riscv/riscv-vector-builtins-bases.cc (enum lst_type): > (enum frm_op_type): move to riscv-vector-builtins-bases.h > * config/riscv/riscv-vector-builtins-bases.h > (GCC_RISCV_VECTOR_BUILTINS_BASES_H): Add header files. > (enum lst_type): move from > (enum frm_op_type): riscv-vector-builtins-bases.cc > (fold_fault_load): riscv-vector-builtins-bases.cc I'm largely hoping to leave the heavy review lifting here to Juzhe who knows GCC's RV vector bits as well as anyone. Just one small issue. Would it be better to prototype fold_fault_load elsewhere and avoid the gimple.h inclusion in riscv-vector-builtins-bases.h? Perhaps riscv-protos.h? You might consider prefixing the function name with riscv_. It's not strictly necessary, but it appears to be relatively common in risc-v port. Thanks, Jeff
Hi Jeff, Perhaps fold_fault_load cannot be moved to riscv-protos.h since gimple_folder is declared in riscv-vector-builtins.h. It's not reasonable to include riscv-vector-builtins.h in riscv-protos.h. In fact, fold_fault_load is defined specially for some builtin functions, and it would be better to just prototype in riscv-vector-builtins-bases.h. Joshua ------------------------------------------------------------------ 发件人：Jeff Law <jeffreyalaw@gmail.com> 发送时间：2023年12月21日(星期四) 02:14 收件人："Jun Sha (Joshua)"<cooper.joshua@linux.alibaba.com>; "gcc-patches"<gcc-patches@gcc.gnu.org> 抄 送："jim.wilson.gcc"<jim.wilson.gcc@gmail.com>; palmer<palmer@dabbelt.com>; andrew<andrew@sifive.com>; "philipp.tomsich"<philipp.tomsich@vrull.eu>; "christoph.muellner"<christoph.muellner@vrull.eu>; "juzhe.zhong"<juzhe.zhong@rivai.ai>; Jin Ma<jinma@linux.alibaba.com>; Xianmiao Qu<cooper.qu@linux.alibaba.com> 主 题：Re: [PATCH v3 1/6] RISC-V: Refactor riscv-vector-builtins-bases.cc On 12/20/23 05:25, Jun Sha (Joshua) wrote: > This patch moves the definition of the enums lst_type and > frm_op_type into riscv-vector-builtins-bases.h and removes > the static visibility of fold_fault_load(), so these > can be used in other compile units. > > gcc/ChangeLog: > > * config/riscv/riscv-vector-builtins-bases.cc (enum lst_type): > (enum frm_op_type): move to riscv-vector-builtins-bases.h > * config/riscv/riscv-vector-builtins-bases.h > (GCC_RISCV_VECTOR_BUILTINS_BASES_H): Add header files. > (enum lst_type): move from > (enum frm_op_type): riscv-vector-builtins-bases.cc > (fold_fault_load): riscv-vector-builtins-bases.cc I'm largely hoping to leave the heavy review lifting here to Juzhe who knows GCC's RV vector bits as well as anyone. Just one small issue. Would it be better to prototype fold_fault_load elsewhere and avoid the gimple.h inclusion in riscv-vector-builtins-bases.h? Perhaps riscv-protos.h? You might consider prefixing the function name with riscv_. It's not strictly necessary, but it appears to be relatively common in risc-v port. Thanks, Jeff
diff --git a/gcc/config/riscv/riscv-vector-builtins-bases.cc b/gcc/config/riscv/riscv-vector-builtins-bases.cc index d70468542ee..c51affde353 100644 --- a/gcc/config/riscv/riscv-vector-builtins-bases.cc +++ b/gcc/config/riscv/riscv-vector-builtins-bases.cc @@ -48,24 +48,8 @@ using namespace riscv_vector; namespace riscv_vector { -/* Enumerates types of loads/stores operations. - It's only used in here so we don't define it - in riscv-vector-builtins-bases.h. */ -enum lst_type -{ - LST_UNIT_STRIDE, - LST_STRIDED, - LST_INDEXED, -}; - -enum frm_op_type -{ - NO_FRM, - HAS_FRM, -}; - /* Helper function to fold vleff and vlsegff. */ -static gimple * +gimple * fold_fault_load (gimple_folder &f) { /* fold fault_load (const *base, size_t *new_vl, size_t vl) diff --git a/gcc/config/riscv/riscv-vector-builtins-bases.h b/gcc/config/riscv/riscv-vector-builtins-bases.h index 131041ea66f..42d0cd17dc1 100644 --- a/gcc/config/riscv/riscv-vector-builtins-bases.h +++ b/gcc/config/riscv/riscv-vector-builtins-bases.h @@ -21,8 +21,27 @@ #ifndef GCC_RISCV_VECTOR_BUILTINS_BASES_H #define GCC_RISCV_VECTOR_BUILTINS_BASES_H +#include "gimple.h" +#include "riscv-vector-builtins.h" + namespace riscv_vector { +/* Enumerates types of loads/stores operations. */ +enum lst_type +{ + LST_UNIT_STRIDE, + LST_STRIDED, + LST_INDEXED, +}; + +enum frm_op_type +{ + NO_FRM, + HAS_FRM, +}; + +extern gimple *fold_fault_load (gimple_folder &f); + namespace bases { extern const function_base *const vsetvl; extern const function_base *const vsetvlmax;