Re: [PATCH v2 2/3] or1k: Add hard float support

[Stafford Horne <shorne@gmail.com>]

On Thu, May 02, 2024 at 03:44:21PM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 29/04/24 02:47, Stafford Horne wrote:
> > This patch adds hardware floating point support to OpenRISC.  Hardware
> > floating point toolchain builds are enabled by passing the machine
> > specific argument -mhard-float to gcc via CFLAGS.  With this enabled GCC
> > generates floating point instructions for single-precision operations
> > and exports __or1k_hard_float__.
> > 
> > There are 2 main parts to this patch.
> > 
> >  - Implement fenv functions to update the FPCSR flags keeping it in sync
> >    with sfp (software floating point).
> >  - Update machine context functions to store and restore the FPCSR
> >    state.
> > 
> > *On mcontext_t ABI*
> > 
> > This patch adds __fpcsr to mcontext_t.  This is an ABI change, but also
> > an ABI fix.  The Linux kernel has always defined padding in mcontext_t
> > that space was missing from the glibc ABI.  In Linux this unused space
> > has now been re-purposed for storing the FPCSR.  This patch brings
> > OpenRISC glibc in line with the Linux kernel and other libc
> > implementation (musl).
> > 
> > Compatibility getcontext, setcontext, etc symbols have been added to
> > allow for binaries expecting the old ABI to continue to work.
> > 
> > *Hard float ABI*
> > 
> > The calling conventions and types do not change with OpenRISC hard-float
> > so glibc hard-float builds continue to use dynamic linker
> > /lib/ld-linux-or1k.so.1.
> > 
> > *Testing*
> > 
> > I have tested this patch both with hard-float and soft-float builds and
> > the test results look fine to me.  Results are as follows:
> > 
> > Hard Float
> > 
> >     # failures
> >     FAIL: elf/tst-sprof-basic		(Haven't figured out yet, not related to hard-float)
> >     FAIL: gmon/tst-gmon-pie		(PIE bug in or1k toolchain)
> >     FAIL: gmon/tst-gmon-pie-gprof	(PIE bug in or1k toolchain)
> >     FAIL: iconvdata/iconv-test		(timeout, passed when run manually)
> >     FAIL: nptl/tst-cond24		(Timeout)
> >     FAIL: nptl/tst-mutex10		(Timeout)
> > 
> >     # summary
> > 	  6 FAIL
> >        4289 PASS
> > 	 86 UNSUPPORTED
> > 	 16 XFAIL
> > 	  2 XPASS
> > 
> >     # versions
> >     Toolchain: or1k-smhfpu-linux-gnu
> >     Compiler:  gcc version 14.0.1 20240324 (experimental) [master r14-9649-gbb04a11418f] (GCC)
> >     Binutils:  GNU assembler version 2.42.0 (or1k-smhfpu-linux-gnu) using BFD version (GNU Binutils) 2.42.0.20240324
> >     Linux:     Linux buildroot 6.9.0-rc1-00008-g4dc70e1aadfa #112 SMP Sat Apr 27 06:43:11 BST 2024 openrisc GNU/Linux
> >     Tester:    shorne
> >     Glibc:     2024-04-25 b62928f907 Florian Weimer   x86: In ld.so, diagnose missing APX support in APX-only builds  (origin/master, origin/HEAD)
> > 
> > Soft Float
> > 
> >     # failures
> >     FAIL: elf/tst-sprof-basic
> >     FAIL: gmon/tst-gmon-pie
> >     FAIL: gmon/tst-gmon-pie-gprof
> >     FAIL: nptl/tst-cond24
> >     FAIL: nptl/tst-mutex10
> > 
> >     # summary
> > 	  5 FAIL
> >        4295 PASS
> > 	 81 UNSUPPORTED
> > 	 16 XFAIL
> > 	  2 XPASS
> > 
> >     # versions
> >     Toolchain: or1k-smh-linux-gnu
> >     Compiler:  gcc version 14.0.1 20240324 (experimental) [master r14-9649-gbb04a11418f] (GCC)
> >     Binutils:  GNU assembler version 2.42.0 (or1k-smh-linux-gnu) using BFD version (GNU Binutils) 2.42.0.20240324
> >     Linux:     Linux buildroot 6.9.0-rc1-00008-g4dc70e1aadfa #112 SMP Sat Apr 27 06:43:11 BST 2024 openrisc GNU/Linux
> >     Tester:    shorne
> >     Glibc:     2024-04-25 b62928f907 Florian Weimer   x86: In ld.so, diagnose missing APX support in APX-only builds  (origin/master, origin/HEAD)
> > 
> > Documentation: https://raw.githubusercontent.com/openrisc/doc/master/openrisc-arch-1.4-rev0.pdf
> 
> The patch looks ok regarding the mcontext_t changes, the new compat
> symbol will always be provided (even for non-fp build), but it should
> be ok.  Also, ff I understand correctly, the ISA and ABI was designed 
> is a way it should not matter whether the shared library is built 
> with/without -mhard-float, so there is no need to add extra ldconfig 
> to handle possible mismatches.

Yes.

> LGTM, thanks.  Some minor nits below.

Thanks very much for the review.

...

> > +#define _FPU_CONTROL_H
> > +
> > +#ifndef __or1k_hard_float__
> > +
> > +# define _FPU_RESERVED 0xffffffff
> > +# define _FPU_DEFAULT  0x00000000
> > +# define _FPU_GETCW(cw) (cw) = 0
> > +# define _FPU_SETCW(cw) (void) (cw)
> > +
> > +#else /* __or1k_hard_float__ */
> > +
> > +/* Layout of FPCSR:
> > +
> > + +-----------+----------------------------+-----+----+
> > + |  32 - 12  | 11 10  9  8  7  6  5  4  3 | 2-1 |  0 |
> > + +-----------+----------------------------+-----+----+
> > + |  Reserved | DZ IN IV IX  Z QN SN UN OV | RM  | EE |
> > + +-----------+----------------------------+-----+----+
> > +
> 
> Not sure who useful is this diagram without much detail of what each
> bitfield means.

Let me add some more docs here, the idea is to help explain what the below masks
mean.

> > + */
> > +
> > +# define _FPU_RESERVED 0xfffff000
> > +/* Default: rounding to nearest with exceptions disabled.  */
> > +# define _FPU_DEFAULT  0
> > +/* IEEE: Same as above with exceptions enabled.  */
> > +# define _FPU_IEEE     (_FPU_DEFAULT | 1)
> > +
> > +# define _FPU_FPCSR_RM_MASK (0x3 << 1)
> > +
> > +/* Macros for accessing the hardware control word.  */
> > +# define _FPU_GETCW(cw) __asm__ volatile ("l.mfspr %0,r0,20" : "=r" (cw))
> > +# define _FPU_SETCW(cw) __asm__ volatile ("l.mtspr r0,%0,20" : : "r" (cw))
> > +
> > +#endif /* __or1k_hard_float__ */
> > +

...

> > diff --git a/sysdeps/or1k/sfp-machine.h b/sysdeps/or1k/sfp-machine.h
> > index d17fd37730..e58e683969 100644
> > --- a/sysdeps/or1k/sfp-machine.h
> > +++ b/sysdeps/or1k/sfp-machine.h
> > @@ -36,7 +36,6 @@
> >  #define _FP_MUL_MEAT_DW_Q(R,X,Y)				\
> >    _FP_MUL_MEAT_DW_4_wide(_FP_WFRACBITS_Q,R,X,Y,umul_ppmm)
> >  
> > -
> 
> Spurious new line.

Ok.

Thanks, I will fix these up and post a v3 to the list before pushing the
patches.

-Stafford