| Message ID | alpine.DEB.2.20.1905020024000.4073@tpp.hgst.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libphobos: RISC-V: Fix soft-float build errors with IEEE exception flags | expand |
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.
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
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.
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
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.
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
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
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
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
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
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
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