You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/03/08 14:04:12 UTC
[GitHub] [incubator-nuttx] no1wudi opened a new pull request #5695: arch/risc-v: Save/Load float register in setjmp
no1wudi opened a new pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695
## Summary
## Impact
riscv only
## Testing
rv-virt:ostest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r821723279
##########
File path: libs/libc/machine/risc-v/common/arch_setjmp.S
##########
@@ -18,16 +18,26 @@
#
############################################################################
-#if __riscv_xlen == 64
+#include <nuttx/config.h>
+
+#ifdef CONFIG_ARCH_RV64
# define SZREG 8
# define REG_S sd
# define REG_L ld
-#elif __riscv_xlen == 32
+#elif defined(CONFIG_ARCH_RV32)
# define SZREG 4
# define REG_S sw
# define REG_L lw
-#else
-# error __riscv_xlen must equal 32 or 64
+#endif
+
+#ifdef CONFIG_ARCH_DPFPU
+# define SZFREG 8
+# define FREG_S fsd
+# define FREG_L fld
+#elif defined(CONFIG_ARCH_FPU)
+# define SZFREG 4
+# define FREG_S fsw
+# define FREG_L flw
Review comment:
```suggestion
# define SZFREG 8
# define FREG_S fsd
# define FREG_L fld
#elif defined(CONFIG_ARCH_FPU)
# define SZFREG 4
# define FREG_S fsw
# define FREG_L flw
```
Just for style consistency with above. Or remove 2 spaces above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r822242818
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,25 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
+
+ /* Float callee register : fs0-fs11 */
+#ifdef CONFIG_ARCH_DPFPU
Review comment:
Emm, it's typo, it should be CONFIG_ARCH_QPFPU
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r821858328
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,24 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
+
+ /* Float callee register : fs0-fs11 */
+
+#ifdef CONFIG_ARCH_DPFPU
+ double fregs[12];
+#elif defined(CONFIG_ARCH_FPU)
Review comment:
> How do we handle `CONFIG_ARCH_QPFPU` case?
Yes, since all other place handle CONFIG_ARCH_QPFPU correctly. @no1wudi please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r821858328
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,24 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
+
+ /* Float callee register : fs0-fs11 */
+
+#ifdef CONFIG_ARCH_DPFPU
+ double fregs[12];
+#elif defined(CONFIG_ARCH_FPU)
Review comment:
> How do we handle `CONFIG_ARCH_QPFPU` case?
Yes, since all other place handle CONFIG_ARCH_QPFPU correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r822240815
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,25 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
+
+ /* Float callee register : fs0-fs11 */
+#ifdef CONFIG_ARCH_DPFPU
Review comment:
should we move CONFIG_ARCH_DPFPU to first?
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,25 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
Review comment:
change long to uintptr_t like xcpt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r821751061
##########
File path: libs/libc/machine/risc-v/common/arch_setjmp.S
##########
@@ -18,38 +18,64 @@
#
############################################################################
-#if __riscv_xlen == 64
-# define SZREG 8
+#include <nuttx/config.h>
+
+#ifdef CONFIG_ARCH_RV64
+# define SZREG 8
# define REG_S sd
# define REG_L ld
-#elif __riscv_xlen == 32
-# define SZREG 4
+#elif defined(CONFIG_ARCH_RV32)
+# define SZREG 4
# define REG_S sw
# define REG_L lw
-#else
-# error __riscv_xlen must equal 32 or 64
+#endif
+
+#ifdef CONFIG_ARCH_DPFPU
+# define SZFREG 8
+# define FREG_S fsd
+# define FREG_L fld
+#elif defined(CONFIG_ARCH_FPU)
+# define SZFREG 4
+# define FREG_S fsw
+# define FREG_L flw
Review comment:
Looking into `arch/risc-v/src/common/riscv_fpu.S` and `arch/risc-v/src/common/riscv_exception_common.S` makes me think that we need
```
#if defined(CONFIG_ARCH_DPFPU)
# define SZFREG 8
# define FLOAD fld
# define FSTORE fsd
#elif defined(CONFIG_ARCH_QPFPU)
# define SZFREG 16
# define FLOAD flq
# define FSTORE fsq
#else
# define SZFREG 4
# define FLOAD flw
# define FSTORE fsw
#endif
#ifdef CONFIG_ARCH_RV32
# define SZREG 4
# define REGLOAD lw
# define REGSTORE sw
#else
# define SZREG 8
# define REGLOAD ld
# define REGSTORE sd
#endif
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r821751061
##########
File path: libs/libc/machine/risc-v/common/arch_setjmp.S
##########
@@ -18,38 +18,64 @@
#
############################################################################
-#if __riscv_xlen == 64
-# define SZREG 8
+#include <nuttx/config.h>
+
+#ifdef CONFIG_ARCH_RV64
+# define SZREG 8
# define REG_S sd
# define REG_L ld
-#elif __riscv_xlen == 32
-# define SZREG 4
+#elif defined(CONFIG_ARCH_RV32)
+# define SZREG 4
# define REG_S sw
# define REG_L lw
-#else
-# error __riscv_xlen must equal 32 or 64
+#endif
+
+#ifdef CONFIG_ARCH_DPFPU
+# define SZFREG 8
+# define FREG_S fsd
+# define FREG_L fld
+#elif defined(CONFIG_ARCH_FPU)
+# define SZFREG 4
+# define FREG_S fsw
+# define FREG_L flw
Review comment:
Looking into `arch/risc-v/src/common/riscv_fpu.S` and `arch/risc-v/src/common/riscv_exception_common.S` makes me think that we need
```
#if defined(CONFIG_ARCH_DPFPU)
# define SZFREG 8
# define FLOAD fld
# define FSTORE fsd
#elif defined(CONFIG_ARCH_QPFPU)
# define SZFREG 16
# define FLOAD flq
# define FSTORE fsq
#else
# define SZFREG 4
# define FLOAD flw
# define FSTORE fsw
#endif
#ifdef CONFIG_ARCH_RV32
# define SZREG 4
# define REGLOAD lw
# define REGSTORE sw
#else
# define SZREG 8
# define REGLOAD ld
# define REGSTORE sd
#endif
```
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,24 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
+
+ /* Float callee register : fs0-fs11 */
+
+#ifdef CONFIG_ARCH_DPFPU
+ double fregs[12];
+#elif defined(CONFIG_ARCH_FPU)
Review comment:
How do we handle `CONFIG_ARCH_QPFPU` case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r821799054
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,24 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
+
+ /* Float callee register : fs0-fs11 */
+
+#ifdef CONFIG_ARCH_DPFPU
+ double fregs[12];
+#elif defined(CONFIG_ARCH_FPU)
Review comment:
Maybe we should add
```
#if defined(CONFIG_ARCH_QPFPU)
long double fregs[12];
#elif defined(CONFIG_ARCH_DPFPU)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r822212602
##########
File path: libs/libc/machine/risc-v/common/arch_setjmp.S
##########
@@ -18,38 +18,64 @@
#
############################################################################
-#if __riscv_xlen == 64
-# define SZREG 8
+#include <nuttx/config.h>
+
+#ifdef CONFIG_ARCH_RV64
+# define SZREG 8
# define REG_S sd
# define REG_L ld
-#elif __riscv_xlen == 32
-# define SZREG 4
+#elif defined(CONFIG_ARCH_RV32)
+# define SZREG 4
# define REG_S sw
# define REG_L lw
-#else
-# error __riscv_xlen must equal 32 or 64
+#endif
+
+#ifdef CONFIG_ARCH_DPFPU
+# define SZFREG 8
+# define FREG_S fsd
+# define FREG_L fld
+#elif defined(CONFIG_ARCH_FPU)
+# define SZFREG 4
+# define FREG_S fsw
+# define FREG_L flw
Review comment:
Done, and add some fix to DPFPU and QPFPU handling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] no1wudi commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r822213121
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,24 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
+
+ /* Float callee register : fs0-fs11 */
+
+#ifdef CONFIG_ARCH_DPFPU
+ double fregs[12];
+#elif defined(CONFIG_ARCH_FPU)
Review comment:
> BTW why do we store only 12 values? From `arch/risc-v/src/common/riscv_fpu.S` I see that there are 32 values? Are 20 left values preserved somehow?
Only fs0-fs11 marked as `Callee`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r821806350
##########
File path: arch/risc-v/include/setjmp.h
##########
@@ -25,13 +25,24 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+#include <nuttx/compiler.h>
+
/****************************************************************************
* Public Types
****************************************************************************/
struct setjmp_buf_s
{
long regs[14];
+
+ /* Float callee register : fs0-fs11 */
+
+#ifdef CONFIG_ARCH_DPFPU
+ double fregs[12];
+#elif defined(CONFIG_ARCH_FPU)
Review comment:
BTW why do we store only 12 values? From `arch/risc-v/src/common/riscv_fpu.S` I see that there are 32 values? Are 20 left values preserved somehow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5695: arch/risc-v: Save/Load float register in setjmp
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5695:
URL: https://github.com/apache/incubator-nuttx/pull/5695#discussion_r821713089
##########
File path: libs/libc/machine/risc-v/common/arch_setjmp.S
##########
@@ -18,16 +18,26 @@
#
############################################################################
-#if __riscv_xlen == 64
+#include <nuttx/config.h>
+
+#ifdef CONFIG_ARCH_RV64
# define SZREG 8
# define REG_S sd
# define REG_L ld
-#elif __riscv_xlen == 32
+#elif defined(CONFIG_ARCH_RV32)
# define SZREG 4
# define REG_S sw
# define REG_L lw
-#else
-# error __riscv_xlen must equal 32 or 64
+#endif
+
+#ifdef CONFIG_ARCH_DPFPU
+# define SZFREG 8
Review comment:
```suggestion
# define SZFREG 8
```
##########
File path: libs/libc/machine/risc-v/common/arch_setjmp.S
##########
@@ -18,16 +18,26 @@
#
############################################################################
-#if __riscv_xlen == 64
+#include <nuttx/config.h>
+
+#ifdef CONFIG_ARCH_RV64
# define SZREG 8
# define REG_S sd
# define REG_L ld
-#elif __riscv_xlen == 32
+#elif defined(CONFIG_ARCH_RV32)
# define SZREG 4
# define REG_S sw
# define REG_L lw
-#else
-# error __riscv_xlen must equal 32 or 64
+#endif
+
+#ifdef CONFIG_ARCH_DPFPU
+# define SZFREG 8
+# define FREG_S fsd
+# define FREG_L fld
+#elif defined(CONFIG_ARCH_FPU)
+# define SZFREG 4
Review comment:
```suggestion
# define SZFREG 4
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org