Thanks Kito and Jeff for comments, will double check and address the comment in v2.
Pan -----Original Message----- From: Kito Cheng <kito.ch...@gmail.com> Sent: Monday, August 21, 2023 11:07 PM To: Jeff Law <jeffreya...@gmail.com> Cc: Li, Pan2 <pan2...@intel.com>; gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; Wang, Yanzhang <yanzhang.w...@intel.com> Subject: Re: [PATCH v1] RISC-V: Refactor RVV class by frm_op_type template arg Just one nit from me: plz add assertion to OP_TYPE_vx to make sure NO FRM_OP == HAS_FRM there On Mon, Aug 21, 2023 at 11:04 PM Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > On 8/17/23 20:53, Pan Li via Gcc-patches wrote: > > From: Pan Li <pan2...@intel.com> > > > > As suggested by kito, we will add new frm_opt_type template arg > > to the op class, to avoid the duplicated function expand. > > > > Signed-off-by: Pan Li <pan2...@intel.com> > > > > gcc/ChangeLog: > > > > * config/riscv/riscv-vector-builtins-bases.cc > > (class binop_frm): Removed. > > (class reverse_binop_frm): Ditto. > > (class widen_binop_frm): Ditto. > > (class vfmacc_frm): Ditto. > > (class vfnmacc_frm): Ditto. > > (class vfmsac_frm): Ditto. > > (class vfnmsac_frm): Ditto. > > (class vfmadd_frm): Ditto. > > (class vfnmadd_frm): Ditto. > > (class vfmsub_frm): Ditto. > > (class vfnmsub_frm): Ditto. > > (class vfwmacc_frm): Ditto. > > (class vfwnmacc_frm): Ditto. > > (class vfwmsac_frm): Ditto. > > (class vfwnmsac_frm): Ditto. > > (class unop_frm): Ditto. > > (class vfrec7_frm): Ditto. > > (class binop): Add frm_op_type template arg. > > (class unop): Ditto. > > (class widen_binop): Ditto. > > (class widen_binop_fp): Ditto. > > (class reverse_binop): Ditto. > > (class vfmacc): Ditto. > > (class vfnmsac): Ditto. > > (class vfmadd): Ditto. > > (class vfnmsub): Ditto. > > (class vfnmacc): Ditto. > > (class vfmsac): Ditto. > > (class vfnmadd): Ditto. > > (class vfmsub): Ditto. > > (class vfwmacc): Ditto. > > (class vfwnmacc): Ditto. > > (class vfwmsac): Ditto. > > (class vfwnmsac): Ditto. > > (class float_misc): Ditto. > So in the expand method, you added a case for OP_TYPE_vx. I assume that > was intentional -- but it's not mentioned anywhere in the ChangeLog. So > please update the ChangeLog if it was intentional or remove the change > if it wasn't intentional. Pre-approved with whichever change is > appropriate. > > Thanks, > Jeff
