Hi,
On 2025-09-30 17:40, Vineet Gupta wrote:
>
>
> On 9/26/25 10:01, Aurelien Jarno wrote:
> > __builtin_round() fails to correctly generate invalid exceptions for NaN
> > inputs when -ftrapping-math is used (which is the default). According to
> > the specification, an invalid exception should be raised for sNaN, but
> > not for qNaN.
> >
> > Commit f12a27216952 ("RISC-V: fix __builtin_round clobbering FP...")
> > attempted to avoid raising an invalid exception for qNaN by saving and
> > restoring the FP exception flags. However this inadvertently suppressed
> > the invalid exception for sNaN as well.
> >
> > Instead of saving/restoring fflags, this patch uses the same approach
> > than the well tested GLIBC round implementation. When flag_trapping_math
> > is enabled, it first checks whether the input is a NaN using feq.s/d. In
> > that case it adds the input value with itself to possibly convert sNaN
> > into qNaN. With this change, the glibc testsuite passes again.
> >
> > The generated code with -ftrapping-math now looks like:
> >
> > convert_float_to_float_round
> > feq.s a5,fa0,fa0
> > beqz a5,.L6
> > auipc a5,0x0
> > flw fa4,42(a5)
> > fabs.s fa5,fa0
> > flt.s a5,fa5,fa4
> > beqz a5,.L5
> > fcvt.w.s a5,fa0,rmm
> > fcvt.s.w fa5,a5
> > fsgnj.s fa0,fa5,fa0
> > ret
> > .L6:
> > fadd.s fa0,fa0,fa0
> > .L5:
> > ret
> >
> > With -fno-trapping-math, the additional checks are omitted so the
> > resulting code is unchanged.
> >
> > Fixes: f652a35877e3 ("This is almost exclusively Jivan's work....")
> > Fixes: f12a27216952 ("RISC-V: fix __builtin_round clobbering FP...")
> >
> > PR target/161652
> >
> > gcc/ChangeLog:
> >
> > * config/riscv/riscv.md (round_pattern): special case NaN input
> > instead of saving/restoring fflags.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/riscv/rvv/autovec/vls/math-nearbyint-1.c: Adjust
> > scan pattern for fewer instances of frflags/fsrflags.
> >
> > Signed-off-by: Aurelien Jarno <[email protected]>
>
> What kind of testing was done on this ?
I have done a native build with --disable-bootstrap and ran the gcc
testsuite looking for regressions. I have then built glibc and ran the
its testsuite to confirm that the issue is fixed and that it doesn't
introduced any new issue.
> You may wanna look at CI pre-commit trigger for this patch [1]
> It shows some lint fixes to address and more importantly shows additional ICE
> in
> fortran.
Thanks, I wasn't aware of this CI pre-commit trigger. And my local tests
predate this new unit test (building and testing takes time!).
Looking at the report, I have noticed that the patch indeed *introduced*
an ICE for gfortran.dg/pr87908.f90 for some targets and optimisation
levels, but at the same time it *fixed* the ICE for the same test and
for different targets and optimisation level. It looks like more a flaky
test than a real issue introduced by that bug.
This seems to be confirmed by the fact this test has been removed in
commit 25f7f04e due to "memory leaks in f951".
For the lint issues, I'll send a new version.
Regards
Aurelien
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
[email protected] http://aurel32.net
