On 5/10/25 07:27, Jeff Law wrote:
>
> On 5/9/25 2:27 PM, Vineet Gupta wrote:
>> Stumbled upon this when trying to wholesale rewrite frm switching code
>> and seeing what pieces needed to be retained from current implementation.
>>
>> My interpretation of how this hook worked, for the following case:
>>
>> fsrmi 3
>> fsrm a4
>> call
>> frrm a4
>> fsrmi 1
>>
>> TARGET_MODE_NEEDED(call_insn) returns DYN_EXIT (to generate fsrm) and
>> TARGET_MODE_AFTER(call_insn) returns DYN (to generate frrm). However
>> for a given insn, if the 2 hooks return different values, the final
>> state machine doesn't switch as expected above (and instead both NEEDED
>> and AFTER need to return the same mode, for most cases).
>>
>> Anyhow it turns out that no-oping this (return the last_mode back) doesn't
>> change any testcase outcomes. There's no change to total number of FRM
>> read/writes emitted (static count) for SPEC2017 -Ofast -march=rv64gcv build
>> But we win again on reduced complexity and maintenance.
>>
>> gcc/ChangeLog:
>>
>> * config/riscv/riscv.cc (riscv_frm_mode_needed): Move static
>> state update here.
>> (frm_unknown_dynamic_p): Delete.
>> (riscv_frm_mode_after): Delete.
>> (riscv_mode_after): Remove call to riscv_frm_mode_after ().
> This doesn't seem right to me.
>
> Don't we need to know when an insn sets FRM to an unknown value, either
> due to a call or due to an explicit assignment to FRM from a GPR?
>
> I suspect that you don't see a difference when you nop this out because
> spec doesn't inherently do FRM mode changes and we probably have lousy
> coverage in the testsuite.
You are right, did I mention always :-)
So indeed testsuite coverage is lousy. I added the hook back and added a debug
print to check if it was returning a mode different than it was passed -
implying it was necessary. For FRM, it hit for a grand total of one time in
entire testsuite for
gcc/testsuite/gcc.target/riscv/rvv/base/float-point-frm-insert-9.c
That test behaves the same w/ or w/o MODE_AFTER, however I was able to tweak it
a little to show that
1. TARGET_MODE_AFTER is needed
2. It is not doing the right thing and needs fixing (New PR/120263)
Thx,
-Vineet
