This showed up when debugging the testcase for PR119164.
RISC-V FRM mode-switching state machine has special handling for transitions
to and from a call_insn as FRM needs to saved/restored around calls
despite it not being a callee-saved reg; rather it's a "global" reg
which can be temporarily modified "locally" with a static RM.
Thus a call needs to see the prior global state, hence the restore (from a
prior backup) before the call. Corollarily any call can potentially clobber
the FRM, thus post-call it needs to be it needs to be re-read/saved.
The following example demostrate this:
- insns 2, 4, 6 correspond to actual user code,
- rest 1, 3, 5, 6 are frm save/restore insns generated by mode switch for the
above described ABI semantics.
test_float_point_frm_static:
1: frrm a5 <--
2: fsrmi 2
3: fsrm a5 <--
4: call normalize_vl
5: frrm a5 <--
6: fsrmi 3
7: fsrm a5 <--
Current implementation of RISC-V TARGET_MODE_NEEDED has special handling
if the call_insn is last insn of BB, to ensure FRM save/reads are emitted
on all the edges. However it doesn't work as intended and is borderline
bogus for following reasons:
- It fails to detect call_insn as last of BB (PR119164 test) if the
next BB starts with a code label (say due to call being conditional).
Granted this is a deficiency of API next_nonnote_nondebug_insn_bb ()
which incorrectly returns next BB code_label as opposed to returning
NULL (and this behavior is kind of relied upon by much of gcc).
This causes missed/delayed state transition to DYN.
- If code is tightened to actually detect above such as:
- rtx_insn *insn = next_nonnote_nondebug_insn_bb (cur_insn);
- if (!insn)
+ if (BB_END (BLOCK_FOR_INSN (cur_insn)) == cur_insn)
edge insertion happens but ends up splitting the BB which generic
mode-sw doesn't expect and ends up hittng an ICE.
- TARGET_MODE_NEEDED hook typically don't modify the CFG.
- For abnormal edges, insert_insn_end_basic_block () is called, which
by design on encountering call_insn as last in BB, inserts new insn
BEFORE the call, not after.
So this is just all wrong and ripe for removal. Moreover there seems to
be no testsuite coverage for this code path at all. Results don't change
at all if this is removed.
The total number of FRM read/writes emitted (static count) across all
benchmarks of a SPEC2017 -Ofast -march=rv64gcv build decrease slightly
so its a net win even if minimal but the real gain is reduced complexity
and maintenance.
Before Patch
---------------- ---------------
frrm fsrmi fsrm frrm fsrmi frrm
perlbench_r 42 0 4 42 0 4
cpugcc_r 167 0 17 167 0 17
bwaves_r 16 0 1 16 0 1
mcf_r 11 0 0 11 0 0
cactusBSSN_r 79 0 27 76 0 27
namd_r 119 0 63 119 0 63
parest_r 218 0 114 168 0 114 <--
povray_r 123 1 17 123 1 17
lbm_r 6 0 0 6 0 0
omnetpp_r 17 0 1 17 0 1
wrf_r 2287 13 1956 2287 13 1956
cpuxalan_r 17 0 1 17 0 1
ldecod_r 11 0 0 11 0 0
x264_r 14 0 1 14 0 1
blender_r 724 12 182 724 12 182
cam4_r 324 13 169 324 13 169
deepsjeng_r 11 0 0 11 0 0
imagick_r 265 16 34 265 16 34
leela_r 12 0 0 12 0 0
nab_r 13 0 1 13 0 1
exchange2_r 16 0 1 16 0 1
fotonik3d_r 20 0 11 20 0 11
roms_r 33 0 23 33 0 23
xz_r 6 0 0 6 0 0
---------------- ---------------
4551 55 2623 4498 55 2623
gcc/ChangeLog:
* config/riscv/riscv.cc (riscv_frm_emit_after_bb_end): Delete.
(riscv_frm_mode_needed): Remove call riscv_frm_emit_after_bb_end.
Signed-off-by: Vineet Gupta <vine...@rivosinc.com>
---
gcc/config/riscv/riscv.cc | 44 ---------------------------------------
1 file changed, 44 deletions(-)
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index d032578f19a4..a1bb51af2be4 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -12318,43 +12318,6 @@ riscv_frm_adjust_mode_after_call (rtx_insn *cur_insn,
int mode)
return mode;
}
-/* Insert the backup frm insn to the end of the bb if and only if the call
- is the last insn of this bb. */
-
-static void
-riscv_frm_emit_after_bb_end (rtx_insn *cur_insn)
-{
- edge eg;
- bool abnormal_edge_p = false;
- edge_iterator eg_iterator;
- basic_block bb = BLOCK_FOR_INSN (cur_insn);
-
- FOR_EACH_EDGE (eg, eg_iterator, bb->succs)
- {
- if (eg->flags & EDGE_ABNORMAL)
- abnormal_edge_p = true;
- else
- {
- start_sequence ();
- emit_insn (gen_frrmsi (DYNAMIC_FRM_RTL (cfun)));
- rtx_insn *backup_insn = end_sequence ();
-
- insert_insn_on_edge (backup_insn, eg);
- }
- }
-
- if (abnormal_edge_p)
- {
- start_sequence ();
- emit_insn (gen_frrmsi (DYNAMIC_FRM_RTL (cfun)));
- rtx_insn *backup_insn = end_sequence ();
-
- insert_insn_end_basic_block (backup_insn, bb);
- }
-
- commit_edge_insertions ();
-}
-
/* Return mode that frm must be switched into
prior to the execution of insn. */
@@ -12369,14 +12332,7 @@ riscv_frm_mode_needed (rtx_insn *cur_insn, int code)
}
if (CALL_P (cur_insn))
- {
- rtx_insn *insn = next_nonnote_nondebug_insn_bb (cur_insn);
-
- if (!insn)
- riscv_frm_emit_after_bb_end (cur_insn);
-
return riscv_vector::FRM_DYN_CALL;
- }
int mode = code >= 0 ? get_attr_frm_mode (cur_insn) : riscv_vector::FRM_NONE;
--
2.43.0
