Thanks Robin for comments. Yes, you can reference this link to compare the difference between gcc and llvm. And I am trying to understand and send the V8 later.
https://godbolt.org/z/4E434vaqv Pan From: juzhe.zhong <juzhe.zh...@rivai.ai> Sent: Wednesday, July 26, 2023 9:13 PM To: Robin Dapp <rdapp....@gmail.com> Cc: Kito Cheng <kito.ch...@sifive.com>; Li, Pan2 <pan2...@intel.com>; rdapp....@gmail.com; gcc-patches@gcc.gnu.org; Wang, Yanzhang <yanzhang.w...@intel.com> Subject: Re: [PATCH v7] RISC-V: Support CALL for RVV floating-point dynamic rounding current llvm didn't do any pre optimization. They always backup+restore for each rounding mode intrinsic We should not reference current llvm ---- Replied Message ---- From Robin Dapp<rdapp....@gmail.com><mailto:rdapp....@gmail.com> Date 07/26/2023 21:08 To Kito Cheng<kito.ch...@sifive.com><mailto:kito.ch...@sifive.com>, Li, Pan2<pan2...@intel.com><mailto:pan2...@intel.com> Cc rdapp....@gmail.com<rdapp....@gmail.com><mailto:rdapp....@gmail.com>, gcc-patches@gcc.gnu.org<gcc-patches@gcc.gnu.org><mailto:gcc-patches@gcc.gnu.org>, juzhe.zh...@rivai.ai<juzhe.zh...@rivai.ai><mailto:juzhe.zh...@rivai.ai>, Wang, Yanzhang<yanzhang.w...@intel.com><mailto:yanzhang.w...@intel.com> Subject Re: [PATCH v7] RISC-V: Support CALL for RVV floating-point dynamic rounding So after thinking about it again - I'm still not really sure I like treating every function as essentially an fesetround. There is a reason why fesetround is special. Does LLVM behave the same way? But supposing we really, really want it and assuming there's consensus: + start_sequence (); + emit_insn (gen_frrmsi (DYNAMIC_FRM_RTL (cfun))); + rtx_insn *backup_insn = get_insns (); + end_sequence (); A comment here would be nice why we need a sequence for a single instruction. I'm not fully aware what insert_insn_end_basic_block does but won't a rtx_insn *last = BB_END (bb); emit_insn_before_noloc (gen_frrmsi (DYNAMIC_FRM_RTL (cfun)), last, bb); suffice? One way or another need these kinds of non-local constructs here don't seem entirely rock solid. @@ -7843,6 +7946,11 @@ riscv_vxrm_mode_after (rtx_insn *insn, int mode) static int riscv_frm_mode_after (rtx_insn *insn, int mode) { + STATIC_FRM_P (cfun) = STATIC_FRM_P (cfun) || riscv_static_frm_mode_p (mode); + + if (CALL_P (insn)) + return FRM_MODE_DYN_CALL; Why do we appear to return a different mode here? We already request FRM_MODE_DYN_CALL in mode_needed. It looks like in the whole function we do not change the mode so we could just always return the incoming mode? This is not part of this patch but related and originally I assumed that we would untangle things after the initial patch, so: if (frm_unknown_dynamic_p (insn)) return FRM_MODE_DYN; frm_unknown_dynamic_p checks CALL_P which has already been checked before. It returns FRM_MODE_DYN instead of FRM_MODE_DYN_CALL, though. Apart from that, the function is called unknown_dynamic but we check for a SET of FRM? Wouldn't something that sets FRM rather be a "static" rounding-mode instruction? (using the "static" wording from before) Then we also still have if (reg_mentioned_p (gen_rtx_REG (SImode, FRM_REGNUM), PATTERN (insn))) return get_attr_frm_mode (insn); from before. Isn't that pretty much the same? + assert_equal (NEW_FRM, get_frm (), + "The value of frm register should be NEW_FRM."); Here and in similar cases, NEW_FRM is not exactly telling. Can't we use "should be " and then + fprintf (stdout, "%s %d, but get %d != %d\n", message, a, b); or similar? + will do the mode switch from MODE_CALL to MODE_NON_NONE natively. NON -> FRM. +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/float-point-dynamic-frm-46.c @@ -0,0 +1,35 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gcv -mabi=lp64 -O3 -Wno-psabi" } */ + +#include "riscv_vector.h" This causes a FAIL for me. I believe the scan directives are off by one. Are you going to do asm directives in a separate patch? Similar to vxrm_unknown_p we could just check for one here and handle it similarly to a call. Would need some more tests, though. Regards Robin
