Re: [PATCH v7] RISC-V: Support CALL for RVV floating-point dynamic rounding

[Robin Dapp <rdapp.gcc@gmail.com>]

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