libphobos: RISC-V: Fix soft-float build errors with IEEE exception flags

Message ID alpine.DEB.2.20.1905020024000.4073@tpp.hgst.com
State New
Headers show
Series
  • libphobos: RISC-V: Fix soft-float build errors with IEEE exception flags
Related show

Commit Message

Maciej Rozycki May 2, 2019, 1:14 a.m.
From: Maciej W. Rozycki <macro@wdc.com>


Fix assembly errors:

.../libphobos/src/std/math.d: Assembler messages:.../libphobos/src/std/math.d:4773: Error: unrecognized opcode `frflags a0'
.../libphobos/src/std/math.d:4856: Error: unrecognized opcode `fsflags a5'
.../libphobos/src/std/math.d:4856: Error: unrecognized opcode `fsflags a5'
.../libphobos/src/std/math.d:4773: Error: unrecognized opcode `frflags a0'
.../libphobos/src/std/math.d:5549: Error: unrecognized opcode `fscsr a5'
.../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a5'
.../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a5'
.../libphobos/src/std/math.d:5549: Error: unrecognized opcode `fscsr a5'
.../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a5'
.../libphobos/src/std/math.d:5549: Error: unrecognized opcode `fscsr a0'
.../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a0'
.../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a0'
.../libphobos/src/std/math.d:5549: Error: unrecognized opcode `fscsr s2'
make[8]: *** [Makefile:1119: std/math.lo] Error 1

triggered with the RISC-V lp64 multilib in a GCC build configured with 
`--enable-multilib --enable-languages=all --target=riscv64-linux-gnu'. 
This is due to unconditional explicit use of F extension instructions 
within inline assembly, to access IEEE exception flags.  The use of 
these instructions is not allowed when building for a soft-float ABI.

Correct the problem by wrapping said inline assembly into a conditional 
such that if `D_SoftFloat' is true, then reads from IEEE exception flags 
return 0 and writes are ignored instead, complementing r270522 
("libphobos: Add D support for RISC-V Linux"), which is an updated 
version of <https://gcc.gnu.org/ml/gcc-patches/2019-04/msg00325.html>, 
where the problematic code has originated from.

	libphobos/
	* std/math.d (IeeeFlags.getIeeeFlags): Handle RISC-V soft-float
	ABI.
	(IeeeFlags.resetIeeeFlags): Likewise.
	(FloatingPointControl.getControlState): Likewise.
	(FloatingPointControl.setControlState): Likewise.
---
Hi,

 I believe this change is obviously correct, and I also verified generated 
code using `objdump -d'.  I have no way to regression-test it right now.

 Please confirm if I correctly referred to identifiers in the ChangeLog 
entry though, as my experience WRT the D programming language and its 
syntax in particular is nil.

 My understanding is changes to `libphobos' are supposed to go upstream 
first, but r270522 is a local change anyway AFAICT, and technically a 
`--enable-languages=all' build regression, so we better fix it ASAP.

 Finally my WDC copyright assignment with FSF is still in the works, but I 
believe this change can be considered legally insignificant for copyright 
purposes, i.e. having at most 15 lines or so, unless adding white space 
for indentation counts against that limit as well (which I doubt).

 With all of the above in mind, OK to apply to trunk and to GCC 9?

  Maciej
---
 libphobos/src/std/math.d |   46 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

gcc-riscv-libphobos-soft-float.diff

Comments

Iain Buclaw May 2, 2019, 7:37 a.m. | #1
On Thu, 2 May 2019 at 03:14, Maciej Rozycki <macro@wdc.com> wrote:
>

> From: Maciej W. Rozycki <macro@wdc.com>

>

> Fix assembly errors:

>

> .../libphobos/src/std/math.d: Assembler messages:.../libphobos/src/std/math.d:4773: Error: unrecognized opcode `frflags a0'

> .../libphobos/src/std/math.d:4856: Error: unrecognized opcode `fsflags a5'

> .../libphobos/src/std/math.d:4856: Error: unrecognized opcode `fsflags a5'

> .../libphobos/src/std/math.d:4773: Error: unrecognized opcode `frflags a0'

> .../libphobos/src/std/math.d:5549: Error: unrecognized opcode `fscsr a5'

> .../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a5'

> .../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a5'

> .../libphobos/src/std/math.d:5549: Error: unrecognized opcode `fscsr a5'

> .../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a5'

> .../libphobos/src/std/math.d:5549: Error: unrecognized opcode `fscsr a0'

> .../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a0'

> .../libphobos/src/std/math.d:5456: Error: unrecognized opcode `frcsr a0'

> .../libphobos/src/std/math.d:5549: Error: unrecognized opcode `fscsr s2'

> make[8]: *** [Makefile:1119: std/math.lo] Error 1

>

> triggered with the RISC-V lp64 multilib in a GCC build configured with

> `--enable-multilib --enable-languages=all --target=riscv64-linux-gnu'.

> This is due to unconditional explicit use of F extension instructions

> within inline assembly, to access IEEE exception flags.  The use of

> these instructions is not allowed when building for a soft-float ABI.

>

> Correct the problem by wrapping said inline assembly into a conditional

> such that if `D_SoftFloat' is true, then reads from IEEE exception flags

> return 0 and writes are ignored instead, complementing r270522

> ("libphobos: Add D support for RISC-V Linux"), which is an updated

> version of <https://gcc.gnu.org/ml/gcc-patches/2019-04/msg00325.html>,

> where the problematic code has originated from.

>

>         libphobos/

>         * std/math.d (IeeeFlags.getIeeeFlags): Handle RISC-V soft-float

>         ABI.

>         (IeeeFlags.resetIeeeFlags): Likewise.

>         (FloatingPointControl.getControlState): Likewise.

>         (FloatingPointControl.setControlState): Likewise.

> ---

> Hi,

>

>  I believe this change is obviously correct, and I also verified generated

> code using `objdump -d'.  I have no way to regression-test it right now.

>

>  Please confirm if I correctly referred to identifiers in the ChangeLog

> entry though, as my experience WRT the D programming language and its

> syntax in particular is nil.

>

>  My understanding is changes to `libphobos' are supposed to go upstream

> first, but r270522 is a local change anyway AFAICT, and technically a

> `--enable-languages=all' build regression, so we better fix it ASAP.

>

>  Finally my WDC copyright assignment with FSF is still in the works, but I

> believe this change can be considered legally insignificant for copyright

> purposes, i.e. having at most 15 lines or so, unless adding white space

> for indentation counts against that limit as well (which I doubt).

>

>  With all of the above in mind, OK to apply to trunk and to GCC 9?

>


Hi,

Looks OK to me.

--
Iain.
Jakub Jelinek May 2, 2019, 10:36 a.m. | #2
On Thu, May 02, 2019 at 09:37:36AM +0200, Iain Buclaw wrote:
> >         libphobos/

> >         * std/math.d (IeeeFlags.getIeeeFlags): Handle RISC-V soft-float

> >         ABI.

> >         (IeeeFlags.resetIeeeFlags): Likewise.

> >         (FloatingPointControl.getControlState): Likewise.

> >         (FloatingPointControl.setControlState): Likewise.


> >  I believe this change is obviously correct, and I also verified generated

> > code using `objdump -d'.  I have no way to regression-test it right now.

> >

> >  Please confirm if I correctly referred to identifiers in the ChangeLog

> > entry though, as my experience WRT the D programming language and its

> > syntax in particular is nil.

> >

> >  My understanding is changes to `libphobos' are supposed to go upstream

> > first, but r270522 is a local change anyway AFAICT, and technically a

> > `--enable-languages=all' build regression, so we better fix it ASAP.

> >

> >  Finally my WDC copyright assignment with FSF is still in the works, but I

> > believe this change can be considered legally insignificant for copyright

> > purposes, i.e. having at most 15 lines or so, unless adding white space

> > for indentation counts against that limit as well (which I doubt).

> >

> >  With all of the above in mind, OK to apply to trunk and to GCC 9?

> >

> 

> Looks OK to me.


The change is ok for 9.1 if it can be committed to gcc-9-branch today.

	Jakub
Iain Buclaw May 2, 2019, 4:39 p.m. | #3
On Thu, 2 May 2019 at 12:36, Jakub Jelinek <jakub@redhat.com> wrote:
>

> On Thu, May 02, 2019 at 09:37:36AM +0200, Iain Buclaw wrote:

> > >         libphobos/

> > >         * std/math.d (IeeeFlags.getIeeeFlags): Handle RISC-V soft-float

> > >         ABI.

> > >         (IeeeFlags.resetIeeeFlags): Likewise.

> > >         (FloatingPointControl.getControlState): Likewise.

> > >         (FloatingPointControl.setControlState): Likewise.

>

> > >  I believe this change is obviously correct, and I also verified generated

> > > code using `objdump -d'.  I have no way to regression-test it right now.

> > >

> > >  Please confirm if I correctly referred to identifiers in the ChangeLog

> > > entry though, as my experience WRT the D programming language and its

> > > syntax in particular is nil.

> > >

> > >  My understanding is changes to `libphobos' are supposed to go upstream

> > > first, but r270522 is a local change anyway AFAICT, and technically a

> > > `--enable-languages=all' build regression, so we better fix it ASAP.

> > >

> > >  Finally my WDC copyright assignment with FSF is still in the works, but I

> > > believe this change can be considered legally insignificant for copyright

> > > purposes, i.e. having at most 15 lines or so, unless adding white space

> > > for indentation counts against that limit as well (which I doubt).

> > >

> > >  With all of the above in mind, OK to apply to trunk and to GCC 9?

> > >

> >

> > Looks OK to me.

>

> The change is ok for 9.1 if it can be committed to gcc-9-branch today.

>


Committed to both trunk and gcc-9-branch.


-- 
Iain
Jim Wilson May 2, 2019, 8:35 p.m. | #4
On Wed, May 1, 2019 at 6:14 PM Maciej Rozycki <macro@wdc.com> wrote:
> within inline assembly, to access IEEE exception flags.  The use of

> these instructions is not allowed when building for a soft-float ABI.


Technically it is an architecture issue not an ABI issue.  If you
compile for -march=rv32imac -mabi=ilp32 then you can't use FP
instructions.  If you compile for -march=rv32imafc -mabi=ilp32 then
you can use FP instructions, but we don't use FP regs for argument
passing.  This is similar to the distinction that ARM makes between
the soft and softfp ABIs.  The RISC-V newlib port for instance checks
__riscv_flen to decide whether to use FP instructions.  The predefined
macro __riscv_flen is set to 0 if the target architecture doesn't have
FP registers.  So the choice of using FP instructions depends on the
target architecture, not the target ABI.

I see that gcc/config/riscv/riscv-d.c has
  if (TARGET_HARD_FLOAT)
    d_add_builtin_version ("D_HardFloat");
  else
    d_add_builtin_version ("D_SoftFloat");
so the patch looks OK, because this is checking the architecture not the ABI.

But the arm/arm-d.c file is defining 3 ARM_* macros for FP support,
and long term we might need something similar in the RISC-V D port for
full RISC-V support.  Through for RISC-V it is probably better to have
separate macros for architecture FP support and ABI FP support, each
of which can have 3 values, because you can have for instance an arch
with 64-bit FP support, but an ABI with only 32-bit FP support, and so
worst case we might need to track the arch/abi FP support separately.
This is how it works in the C front end.  We have __riscv_flen which
can be 0, 4, or 8 and which indicates the hardware FP register size in
bytes.  And we have __riscv_float_abi_soft, __riscv_float_abi_single,
and __riscv_float_abi_double, only one of which is defined, which
indicates the max size of arguments that can be passed in FP
registers.

Jim
Iain Buclaw May 3, 2019, 6:40 a.m. | #5
On Thu, 2 May 2019 at 22:35, Jim Wilson <jimw@sifive.com> wrote:
>

> On Wed, May 1, 2019 at 6:14 PM Maciej Rozycki <macro@wdc.com> wrote:

> > within inline assembly, to access IEEE exception flags.  The use of

> > these instructions is not allowed when building for a soft-float ABI.

>

> Technically it is an architecture issue not an ABI issue.  If you

> compile for -march=rv32imac -mabi=ilp32 then you can't use FP

> instructions.  If you compile for -march=rv32imafc -mabi=ilp32 then

> you can use FP instructions, but we don't use FP regs for argument

> passing.  This is similar to the distinction that ARM makes between

> the soft and softfp ABIs.  The RISC-V newlib port for instance checks

> __riscv_flen to decide whether to use FP instructions.  The predefined

> macro __riscv_flen is set to 0 if the target architecture doesn't have

> FP registers.  So the choice of using FP instructions depends on the

> target architecture, not the target ABI.

>

> I see that gcc/config/riscv/riscv-d.c has

>   if (TARGET_HARD_FLOAT)

>     d_add_builtin_version ("D_HardFloat");

>   else

>     d_add_builtin_version ("D_SoftFloat");

> so the patch looks OK, because this is checking the architecture not the ABI.

>

> But the arm/arm-d.c file is defining 3 ARM_* macros for FP support,

> and long term we might need something similar in the RISC-V D port for

> full RISC-V support.  Through for RISC-V it is probably better to have

> separate macros for architecture FP support and ABI FP support, each

> of which can have 3 values, because you can have for instance an arch

> with 64-bit FP support, but an ABI with only 32-bit FP support, and so

> worst case we might need to track the arch/abi FP support separately.

> This is how it works in the C front end.  We have __riscv_flen which

> can be 0, 4, or 8 and which indicates the hardware FP register size in

> bytes.  And we have __riscv_float_abi_soft, __riscv_float_abi_single,

> and __riscv_float_abi_double, only one of which is defined, which

> indicates the max size of arguments that can be passed in FP

> registers.

>


A future version of the D language adds a new feature to cover this,
__traits(getTargetInfo,"key").

https://dlang.org/spec/traits.html#getTargetInfo

I have a slight inclination to push the information there instead of
adding additional RISCV_* version conditions, particularly as the
result returned can be any kind of value, which has a slight leverage
over version conditions which are just boolean true/false.

-- 
Iain
Maciej Rozycki May 3, 2019, 8:05 p.m. | #6
On Thu, 2 May 2019, Jim Wilson wrote:

> > within inline assembly, to access IEEE exception flags.  The use of

> > these instructions is not allowed when building for a soft-float ABI.

> 

> Technically it is an architecture issue not an ABI issue.  If you

> compile for -march=rv32imac -mabi=ilp32 then you can't use FP

> instructions.  If you compile for -march=rv32imafc -mabi=ilp32 then

> you can use FP instructions, but we don't use FP regs for argument

> passing.  This is similar to the distinction that ARM makes between

> the soft and softfp ABIs.  The RISC-V newlib port for instance checks

> __riscv_flen to decide whether to use FP instructions.  The predefined

> macro __riscv_flen is set to 0 if the target architecture doesn't have

> FP registers.  So the choice of using FP instructions depends on the

> target architecture, not the target ABI.


 I found this a useful clarification, thank you!  I wasn't aware of the 
`-march=rv32imafc -mabi=ilp32' hybrid mode.

> This is how it works in the C front end.  We have __riscv_flen which

> can be 0, 4, or 8 and which indicates the hardware FP register size in

> bytes.  And we have __riscv_float_abi_soft, __riscv_float_abi_single,

> and __riscv_float_abi_double, only one of which is defined, which

> indicates the max size of arguments that can be passed in FP

> registers.


 FWIW it makes sense to me, and I find the nomenclature somewhat clearer 
than ARM's `soft' vs `softfp' modes, though I think one still has to refer 
to documentation to understand the distinction between these variants.

  Maciej
Maciej Rozycki May 3, 2019, 10:11 p.m. | #7
On Fri, 3 May 2019, Maciej W. Rozycki wrote:

> > Technically it is an architecture issue not an ABI issue.  If you

> > compile for -march=rv32imac -mabi=ilp32 then you can't use FP

> > instructions.  If you compile for -march=rv32imafc -mabi=ilp32 then

> > you can use FP instructions, but we don't use FP regs for argument

> > passing.  This is similar to the distinction that ARM makes between

> > the soft and softfp ABIs.  The RISC-V newlib port for instance checks

> > __riscv_flen to decide whether to use FP instructions.  The predefined

> > macro __riscv_flen is set to 0 if the target architecture doesn't have

> > FP registers.  So the choice of using FP instructions depends on the

> > target architecture, not the target ABI.

> 

>  I found this a useful clarification, thank you!  I wasn't aware of the 

> `-march=rv32imafc -mabi=ilp32' hybrid mode.


 Hmm, I've been thinking a little bit about this hybrid mode and I have 
one question: how do we pass the IEEE rounding mode setting between `fcsr' 
and softfp where we have `-march=rv32imafc -mabi=ilp32' and 
`-march=rv32imac -mabi=ilp32' object modules interlinked?

  Maciej
Jim Wilson May 6, 2019, 7:03 p.m. | #8
On Fri, May 3, 2019 at 3:11 PM Maciej W. Rozycki <macro@wdc.com> wrote:
>  Hmm, I've been thinking a little bit about this hybrid mode and I have

> one question: how do we pass the IEEE rounding mode setting between `fcsr'

> and softfp where we have `-march=rv32imafc -mabi=ilp32' and

> `-march=rv32imac -mabi=ilp32' object modules interlinked?


If you look at libgcc/config/riscv/sfp-machine.h you will see
definitions of FP_INIT_ROUNDMODE and FP_HANDLE_EXCEPTIONS for reading
rounding mode from the fcsr before soft-float FP operations, and
storing the exception flags back into fcsr after soft-float FP
operations, if FP regs exist..  Whether this actually works, I don't
know, I haven't tested it.  I think in practice people using
soft-float libraries are probably not relying on rounding modes and
exception flags much if at all, so this is unlikely to be a problem.
If someone does find a problem, I will worry about how to fix it then.

Jim
Maciej Rozycki May 9, 2019, 10:22 p.m. | #9
Iain, Jakub --

> > > >  With all of the above in mind, OK to apply to trunk and to GCC 9?

> > >

> > > Looks OK to me.

> >

> > The change is ok for 9.1 if it can be committed to gcc-9-branch today.

> 

> Committed to both trunk and gcc-9-branch.


 Thank you both for the review, however, Iain, in the future, when 
committing on my behalf rather than letting me do it myself, would you 
please do use the commit message I have prepared and be duly dilligent 
with respect to the details I have provided?  Thank you.

 Kind regards,

  Maciej W. Rozycki
Maciej Rozycki May 9, 2019, 11:37 p.m. | #10
On Mon, 6 May 2019, Jim Wilson wrote:

> >  Hmm, I've been thinking a little bit about this hybrid mode and I have

> > one question: how do we pass the IEEE rounding mode setting between `fcsr'

> > and softfp where we have `-march=rv32imafc -mabi=ilp32' and

> > `-march=rv32imac -mabi=ilp32' object modules interlinked?

> 

> If you look at libgcc/config/riscv/sfp-machine.h you will see

> definitions of FP_INIT_ROUNDMODE and FP_HANDLE_EXCEPTIONS for reading

> rounding mode from the fcsr before soft-float FP operations, and

> storing the exception flags back into fcsr after soft-float FP

> operations, if FP regs exist..  Whether this actually works, I don't

> know, I haven't tested it.


 I've looked at the code and it looks like it should do what's intended 
here.

>  I think in practice people using

> soft-float libraries are probably not relying on rounding modes and

> exception flags much if at all, so this is unlikely to be a problem.


 However I gather the intent for the hybrid mode has been to permit people 
to seamlessly interlink hard-float and soft-float code, perhaps libraries 
that have only been supplied in a binary form (whether we like it or not) 
and cannot therefore be rebuilt for the other FP model.  As such I think 
we should ensure that consistent operation has been architected for the 
reasonable scenarios, such that if any problem problem arises, then it's 
at most a bug and not a design fault.  It does appear to me we don't have 
a design fault here, so no need to worry.

> If someone does find a problem, I will worry about how to fix it then.


 Sure.  I just wanted to double-check we don't have an ABI issue here.
 
  Maciej

Patch

Index: gcc/libphobos/src/std/math.d
===================================================================
--- gcc.orig/libphobos/src/std/math.d
+++ gcc/libphobos/src/std/math.d
@@ -4767,12 +4767,17 @@  struct IeeeFlags
             }
             else version (RISCV_Any)
             {
-                uint result = void;
-                asm pure nothrow @nogc
+                version (D_SoftFloat)
+                    return 0;
+                else
                 {
-                    "frflags %0" : "=r" result;
+                    uint result = void;
+                    asm pure nothrow @nogc
+                    {
+                        "frflags %0" : "=r" result;
+                    }
+                    return result;
                 }
-                return result;
             }
             else
                 assert(0, "Not yet supported");
@@ -4850,10 +4855,15 @@  struct IeeeFlags
             }
             else version (RISCV_Any)
             {
-                uint newValues = 0x0;
-                asm pure nothrow @nogc
+                version (D_SoftFloat)
+                    return;
+                else
                 {
-                    "fsflags %0" : : "r" newValues;
+                    uint newValues = 0x0;
+                    asm pure nothrow @nogc
+                    {
+                        "fsflags %0" : : "r" newValues;
+                    }
                 }
             }
             else
@@ -5450,12 +5460,17 @@  struct FloatingPointControl
             }
             else version (RISCV_Any)
             {
-                ControlState cont;
-                asm pure nothrow @nogc
+                version (D_SoftFloat)
+                    return 0;
+                else
                 {
-                    "frcsr %0" : "=r" cont;
+                    ControlState cont;
+                    asm pure nothrow @nogc
+                    {
+                        "frcsr %0" : "=r" cont;
+                    }
+                    return cont;
                 }
-                return cont;
             }
             else
                 assert(0, "Not yet supported");
@@ -5544,9 +5559,14 @@  struct FloatingPointControl
             }
             else version (RISCV_Any)
             {
-                asm pure nothrow @nogc
+                version (D_SoftFloat)
+                    return;
+                else
                 {
-                    "fscsr %0" : : "r" (newState);
+                    asm pure nothrow @nogc
+                    {
+                        "fscsr %0" : : "r" (newState);
+                    }
                 }
             }
             else