[PATCH v2 0/5] RISC-V big endian support

[PATCH v2 0/5] RISC-V big endian support

[Marcus Comstedt]

This is an update to the patch series for big endian RISC-V support.

Changes since last version:

  * Added documentation of -mbig-endian and -mlittle-endian

  * New patch: Fix soft-fp endianness setting

  * New patch: Fix trampoline generation on big endian

  * New patch: Update the shift-shift-5.c testcase to work correctly
    on big endian

With these changes, and two fixes to newlib (setting correct floating
point byteorder, and an update to the handcoded assembler for strcmp),
I'm now down to

               ========= Summary of gcc testsuite =========
                            | # of unexpected case / # of unique unexpected case
                            |          gcc |          g++ |     gfortran |
     rv64gc/   lp64/ medlow |   14 /     8 |   39 /    10 |      - |

and of these only two failures do not also occur for little endian:

FAIL: gcc.target/riscv/shift-and-1.c scan-assembler-not andi
FAIL: gcc.target/riscv/shift-and-2.c scan-assembler-not andi

I'm quite puzzled why these two testcases give different results with
-mbig-endian compared to with -mlittle-endian though, since they only
deal with register-to-register operations so the memory model should be
completely irrelevant...


  // Marcus

[PATCH v2 1/5] RISC-V: Support -mlittle-endian and -mbig-endian

[Marcus Comstedt]

gcc/
	* config/riscv/elf.h (LINK_SPEC): Pass linker endianness flag.
	* config/riscv/freebsd.h (LINK_SPEC): Likewise.
	* config/riscv/linux.h (LINK_SPEC): Likewise.
	* config/riscv/riscv.h (ASM_SPEC): Pass -mbig-endian and
	-mlittle-endian.
	(BYTES_BIG_ENDIAN): Handle big endian.
	(WORDS_BIG_ENDIAN): Define to BYTES_BIG_ENDIAN.
	* config/riscv/riscv.opt (-mbig-endian, -mlittle-endian): New
	options.
	* doc/invoke.texi (-mbig-endian, -mlittle-endian): Document.
---
 gcc/config/riscv/elf.h     |  2 ++
 gcc/config/riscv/freebsd.h |  2 ++
 gcc/config/riscv/linux.h   |  2 ++
 gcc/config/riscv/riscv.h   |  6 ++++--
 gcc/config/riscv/riscv.opt |  8 ++++++++
 gcc/doc/invoke.texi        | 12 ++++++++++++
 6 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/gcc/config/riscv/elf.h b/gcc/config/riscv/elf.h
index d136d46e4fa..973efdaed7b 100644
--- a/gcc/config/riscv/elf.h
+++ b/gcc/config/riscv/elf.h
@@ -20,6 +20,8 @@ along with GCC; see the file COPYING3.  If not see
 #define LINK_SPEC "\
 -melf" XLEN_SPEC "lriscv \
 %{mno-relax:--no-relax} \
+%{mbig-endian:-EB} \
+%{mlittle-endian:-EL} \
 %{shared}"
 
 /* Link against Newlib libraries, because the ELF backend assumes Newlib.
diff --git a/gcc/config/riscv/freebsd.h b/gcc/config/riscv/freebsd.h
index a48bf9bffe4..f3aca9f7673 100644
--- a/gcc/config/riscv/freebsd.h
+++ b/gcc/config/riscv/freebsd.h
@@ -44,6 +44,8 @@ along with GCC; see the file COPYING3.  If not see
   %{p:%nconsider using `-pg' instead of `-p' with gprof (1)}	\
   %{v:-V}							\
   %{assert*} %{R*} %{rpath*} %{defsym*}				\
+  %{mbig-endian:-EB}						\
+  %{mlittle-endian:-EL}						\
   %{shared:-Bshareable %{h*} %{soname*}}			\
   %{symbolic:-Bsymbolic}					\
   %{static:-Bstatic}						\
diff --git a/gcc/config/riscv/linux.h b/gcc/config/riscv/linux.h
index 9238de5bc92..e74f5d3f914 100644
--- a/gcc/config/riscv/linux.h
+++ b/gcc/config/riscv/linux.h
@@ -60,6 +60,8 @@ along with GCC; see the file COPYING3.  If not see
 #define LINK_SPEC "\
 -melf" XLEN_SPEC "lriscv" LD_EMUL_SUFFIX " \
 %{mno-relax:--no-relax} \
+%{mbig-endian:-EB} \
+%{mlittle-endian:-EL} \
 %{shared} \
   %{!shared: \
     %{!static: \
diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index c6f8bee07ef..0b667d2e8b9 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -91,6 +91,8 @@ extern const char *riscv_default_mtune (int argc, const char **argv);
 %{" FPIE_OR_FPIC_SPEC ":-fpic} \
 %{march=*} \
 %{mabi=*} \
+%{mbig-endian} \
+%{mlittle-endian} \
 %(subtarget_asm_spec)" \
 ASM_MISA_SPEC
 
@@ -126,8 +128,8 @@ ASM_MISA_SPEC
 /* Target machine storage layout */
 
 #define BITS_BIG_ENDIAN 0
-#define BYTES_BIG_ENDIAN 0
-#define WORDS_BIG_ENDIAN 0
+#define BYTES_BIG_ENDIAN (TARGET_BIG_ENDIAN != 0)
+#define WORDS_BIG_ENDIAN (BYTES_BIG_ENDIAN)
 
 #define MAX_BITS_PER_WORD 64
 
diff --git a/gcc/config/riscv/riscv.opt b/gcc/config/riscv/riscv.opt
index 761a09d18c3..e294e223151 100644
--- a/gcc/config/riscv/riscv.opt
+++ b/gcc/config/riscv/riscv.opt
@@ -21,6 +21,14 @@
 HeaderInclude
 config/riscv/riscv-opts.h
 
+mbig-endian
+Target RejectNegative Mask(BIG_ENDIAN)
+Assume target CPU is configured as big endian.
+
+mlittle-endian
+Target RejectNegative InverseMask(BIG_ENDIAN)
+Assume target CPU is configured as little endian.
+
 mbranch-cost=
 Target RejectNegative Joined UInteger Var(riscv_branch_cost)
 -mbranch-cost=N	Set the cost of branches to roughly N instructions.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index e8baa545eee..9279a37a832 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -1169,6 +1169,7 @@ See RS/6000 and PowerPC Options.
 -mrelax  -mno-relax @gol
 -mriscv-attribute  -mmo-riscv-attribute @gol
 -malign-data=@var{type} @gol
+-mbig-endian  -mlittle-endian @gol
 +-mstack-protector-guard=@var{guard} -mstack-protector-guard-reg=@var{reg} @gol
 +-mstack-protector-guard-offset=@var{offset}}
 
@@ -26721,6 +26722,17 @@ types.  Supported values for @var{type} are @samp{xlen} which uses x register
 width as the alignment value, and @samp{natural} which uses natural alignment.
 @samp{xlen} is the default.
 
+@item -mbig-endian
+@opindex mbig-endian
+Generate big-endian code.  This is the default when GCC is configured for a
+@samp{riscv64be-*-*} or @samp{riscv32be-*-*} target.
+
+@item -mlittle-endian
+@opindex mlittle-endian
+Generate little-endian code.  This is the default when GCC is configured for a
+@samp{riscv64-*-*} or @samp{riscv32-*-*} but not a @samp{riscv64be-*-*} or
+@samp{riscv32be-*-*} target.
+
 @item -mstack-protector-guard=@var{guard}
 @itemx -mstack-protector-guard-reg=@var{reg}
 @itemx -mstack-protector-guard-offset=@var{offset}
-- 
2.26.2

[PATCH v2 2/5] RISC-V: Add riscv{32,64}be with big endian as default

[Marcus Comstedt]

gcc/
	* common/config/riscv/riscv-common.c
	(TARGET_DEFAULT_TARGET_FLAGS): Set default endianness.
	* config.gcc (riscv32be-*, riscv64be-*): Set
	TARGET_BIG_ENDIAN_DEFAULT to 1.
	* config/riscv/elf.h (LINK_SPEC): Change -melf* value
	depending on default endianness.
	* config/riscv/freebsd.h (LINK_SPEC): Likewise.
	* config/riscv/linux.h (LINK_SPEC): Likewise.
	* config/riscv/riscv.c (TARGET_DEFAULT_TARGET_FLAGS): Set
	default endianness.
	* config/riscv/riscv.h (DEFAULT_ENDIAN_SPEC): New macro.
---
 gcc/common/config/riscv/riscv-common.c |  5 +++++
 gcc/config.gcc                         | 15 +++++++++++++++
 gcc/config/riscv/elf.h                 |  2 +-
 gcc/config/riscv/freebsd.h             |  2 +-
 gcc/config/riscv/linux.h               |  2 +-
 gcc/config/riscv/riscv.c               |  5 +++++
 gcc/config/riscv/riscv.h               |  6 ++++++
 7 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/gcc/common/config/riscv/riscv-common.c b/gcc/common/config/riscv/riscv-common.c
index 6bbe25dba89..34b74e52a2d 100644
--- a/gcc/common/config/riscv/riscv-common.c
+++ b/gcc/common/config/riscv/riscv-common.c
@@ -32,6 +32,11 @@ along with GCC; see the file COPYING3.  If not see
 #include "config/riscv/riscv-protos.h"
 #include "config/riscv/riscv-subset.h"
 
+#ifdef  TARGET_BIG_ENDIAN_DEFAULT
+#undef  TARGET_DEFAULT_TARGET_FLAGS
+#define TARGET_DEFAULT_TARGET_FLAGS (MASK_BIG_ENDIAN)
+#endif
+
 /* Type for implied ISA info.  */
 struct riscv_implied_info_t
 {
diff --git a/gcc/config.gcc b/gcc/config.gcc
index 17fea83b2e4..ae47e430062 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -2464,6 +2464,11 @@ riscv*-*-linux*)
 	tmake_file="${tmake_file} riscv/t-riscv riscv/t-linux"
 	gnu_ld=yes
 	gas=yes
+	case $target in
+	riscv32be-*|riscv64be-*)
+		tm_defines="${tm_defines} TARGET_BIG_ENDIAN_DEFAULT=1"
+		;;
+	esac
 	# Force .init_array support.  The configure script cannot always
 	# automatically detect that GAS supports it, yet we require it.
 	gcc_cv_initfini_array=yes
@@ -2487,6 +2492,11 @@ riscv*-*-elf* | riscv*-*-rtems*)
 	tmake_file="${tmake_file} riscv/t-riscv"
 	gnu_ld=yes
 	gas=yes
+	case $target in
+	riscv32be-*|riscv64be-*)
+		tm_defines="${tm_defines} TARGET_BIG_ENDIAN_DEFAULT=1"
+		;;
+	esac
 	# Force .init_array support.  The configure script cannot always
 	# automatically detect that GAS supports it, yet we require it.
 	gcc_cv_initfini_array=yes
@@ -2496,6 +2506,11 @@ riscv*-*-freebsd*)
 	tmake_file="${tmake_file} riscv/t-riscv"
 	gnu_ld=yes
 	gas=yes
+	case $target in
+	riscv32be-*|riscv64be-*)
+		tm_defines="${tm_defines} TARGET_BIG_ENDIAN_DEFAULT=1"
+		;;
+	esac
 	# Force .init_array support.  The configure script cannot always
 	# automatically detect that GAS supports it, yet we require it.
 	gcc_cv_initfini_array=yes
diff --git a/gcc/config/riscv/elf.h b/gcc/config/riscv/elf.h
index 973efdaed7b..7e65e499031 100644
--- a/gcc/config/riscv/elf.h
+++ b/gcc/config/riscv/elf.h
@@ -18,7 +18,7 @@ along with GCC; see the file COPYING3.  If not see
 <http://www.gnu.org/licenses/>.  */
 
 #define LINK_SPEC "\
--melf" XLEN_SPEC "lriscv \
+-melf" XLEN_SPEC DEFAULT_ENDIAN_SPEC "riscv \
 %{mno-relax:--no-relax} \
 %{mbig-endian:-EB} \
 %{mlittle-endian:-EL} \
diff --git a/gcc/config/riscv/freebsd.h b/gcc/config/riscv/freebsd.h
index f3aca9f7673..6018e7bb764 100644
--- a/gcc/config/riscv/freebsd.h
+++ b/gcc/config/riscv/freebsd.h
@@ -40,7 +40,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #undef LINK_SPEC
 #define LINK_SPEC "						\
-  -melf" XLEN_SPEC "lriscv					\
+  -melf" XLEN_SPEC DEFAULT_ENDIAN_SPEC "riscv			\
   %{p:%nconsider using `-pg' instead of `-p' with gprof (1)}	\
   %{v:-V}							\
   %{assert*} %{R*} %{rpath*} %{defsym*}				\
diff --git a/gcc/config/riscv/linux.h b/gcc/config/riscv/linux.h
index e74f5d3f914..fce5b896e6e 100644
--- a/gcc/config/riscv/linux.h
+++ b/gcc/config/riscv/linux.h
@@ -58,7 +58,7 @@ along with GCC; see the file COPYING3.  If not see
   "%{mabi=ilp32:_ilp32}"
 
 #define LINK_SPEC "\
--melf" XLEN_SPEC "lriscv" LD_EMUL_SUFFIX " \
+-melf" XLEN_SPEC DEFAULT_ENDIAN_SPEC "riscv" LD_EMUL_SUFFIX " \
 %{mno-relax:--no-relax} \
 %{mbig-endian:-EB} \
 %{mlittle-endian:-EL} \
diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index fffd0814eee..eab14602355 100644
--- a/gcc/config/riscv/riscv.c
+++ b/gcc/config/riscv/riscv.c
@@ -5524,6 +5524,11 @@ riscv_asan_shadow_offset (void)
 #undef TARGET_ASAN_SHADOW_OFFSET
 #define TARGET_ASAN_SHADOW_OFFSET riscv_asan_shadow_offset
 
+#ifdef TARGET_BIG_ENDIAN_DEFAULT
+#undef  TARGET_DEFAULT_TARGET_FLAGS
+#define TARGET_DEFAULT_TARGET_FLAGS (MASK_BIG_ENDIAN)
+#endif
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-riscv.h"
diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index 0b667d2e8b9..3cc3e864a3e 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -30,6 +30,12 @@ along with GCC; see the file COPYING3.  If not see
 /* Target CPU versions for D.  */
 #define TARGET_D_CPU_VERSIONS riscv_d_target_versions
 
+#ifdef TARGET_BIG_ENDIAN_DEFAULT
+#define DEFAULT_ENDIAN_SPEC    "b"
+#else
+#define DEFAULT_ENDIAN_SPEC    "l"
+#endif
+
 /* Default target_flags if no switches are specified  */
 
 #ifndef TARGET_DEFAULT
-- 
2.26.2

[PATCH v2 3/5] RISC-V: Update soft-fp config for big-endian

[Marcus Comstedt]

libgcc/
	* config/riscv/sfp-machine.h (__BYTE_ORDER): Set according
	to __BYTE_ORDER__.
---
 libgcc/config/riscv/sfp-machine.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libgcc/config/riscv/sfp-machine.h b/libgcc/config/riscv/sfp-machine.h
index db2697157ce..8adbf4b8b2e 100644
--- a/libgcc/config/riscv/sfp-machine.h
+++ b/libgcc/config/riscv/sfp-machine.h
@@ -128,7 +128,11 @@ do {								\
 #define	__LITTLE_ENDIAN	1234
 #define	__BIG_ENDIAN	4321
 
+#if defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)
+#define __BYTE_ORDER __BIG_ENDIAN
+#else
 #define __BYTE_ORDER __LITTLE_ENDIAN
+#endif
 
 
 /* Define ALIASNAME as a strong alias for NAME.  */
-- 
2.26.2

[PATCH v2 4/5] RISC-V: Fix trampoline generation on big endian

[Marcus Comstedt]

gcc/
	* config/riscv/riscv.c (riscv_swap_instruction): New function
	to byteswap an SImode rtx containing an instruction.
	(riscv_trampoline_init): Byteswap the generated instructions
	when needed.
---
 gcc/config/riscv/riscv.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index eab14602355..1cd795bd19c 100644
--- a/gcc/config/riscv/riscv.c
+++ b/gcc/config/riscv/riscv.c
@@ -1073,6 +1073,15 @@ riscv_force_binary (machine_mode mode, enum rtx_code code, rtx x, rtx y)
   return riscv_emit_binary (code, gen_reg_rtx (mode), x, y);
 }
 
+static rtx
+riscv_swap_instruction (rtx inst)
+{
+  gcc_assert (GET_MODE (inst) == SImode);
+  if (BYTES_BIG_ENDIAN)
+    inst = expand_unop (SImode, bswap_optab, inst, gen_reg_rtx (SImode), 1);
+  return inst;
+}
+
 /* Copy VALUE to a register and return that register.  If new pseudos
    are allowed, copy it into a new register, otherwise use DEST.  */
 
@@ -4953,7 +4962,7 @@ riscv_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
 					     gen_int_mode (lui_hi_chain_code, SImode));
 
       mem = adjust_address (m_tramp, SImode, 0);
-      riscv_emit_move (mem, lui_hi_chain);
+      riscv_emit_move (mem, riscv_swap_instruction (lui_hi_chain));
 
       /* Gen lui t0, hi(func).  */
       rtx hi_func = riscv_force_binary (SImode, PLUS, target_function,
@@ -4965,7 +4974,7 @@ riscv_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
 					    gen_int_mode (lui_hi_func_code, SImode));
 
       mem = adjust_address (m_tramp, SImode, 1 * GET_MODE_SIZE (SImode));
-      riscv_emit_move (mem, lui_hi_func);
+      riscv_emit_move (mem, riscv_swap_instruction (lui_hi_func));
 
       /* Gen addi t2, t2, lo(chain).  */
       rtx lo_chain = riscv_force_binary (SImode, AND, chain_value,
@@ -4980,7 +4989,7 @@ riscv_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
 					      force_reg (SImode, GEN_INT (lo_chain_code)));
 
       mem = adjust_address (m_tramp, SImode, 2 * GET_MODE_SIZE (SImode));
-      riscv_emit_move (mem, addi_lo_chain);
+      riscv_emit_move (mem, riscv_swap_instruction (addi_lo_chain));
 
       /* Gen jr t0, lo(func).  */
       rtx lo_func = riscv_force_binary (SImode, AND, target_function,
@@ -4993,7 +5002,7 @@ riscv_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
 					   force_reg (SImode, GEN_INT (lo_func_code)));
 
       mem = adjust_address (m_tramp, SImode, 3 * GET_MODE_SIZE (SImode));
-      riscv_emit_move (mem, jr_lo_func);
+      riscv_emit_move (mem, riscv_swap_instruction (jr_lo_func));
     }
   else
     {
@@ -5019,6 +5028,8 @@ riscv_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
       /* Copy the trampoline code.  */
       for (i = 0; i < ARRAY_SIZE (trampoline); i++)
 	{
+	  if (BYTES_BIG_ENDIAN)
+	    trampoline[i] = __builtin_bswap32(trampoline[i]);
 	  mem = adjust_address (m_tramp, SImode, i * GET_MODE_SIZE (SImode));
 	  riscv_emit_move (mem, gen_int_mode (trampoline[i], SImode));
 	}
-- 
2.26.2

[PATCH v2 5/5] RISC-V: Update shift-shift-5.c testcase for big endian

[Marcus Comstedt]

gcc/
	* testsuite/gcc.target/riscv/shift-shift-5.c (sub): Change
	order of struct fields depending on byteorder.
---
 gcc/testsuite/gcc.target/riscv/shift-shift-5.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gcc/testsuite/gcc.target/riscv/shift-shift-5.c b/gcc/testsuite/gcc.target/riscv/shift-shift-5.c
index 5b2ae89a471..0ecab9723c9 100644
--- a/gcc/testsuite/gcc.target/riscv/shift-shift-5.c
+++ b/gcc/testsuite/gcc.target/riscv/shift-shift-5.c
@@ -7,7 +7,11 @@ unsigned long
 sub (long l)
 {
   union u {
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
     struct s { int a : 19; unsigned int b : 13; int x; } s;
+#else
+    struct s { int x; unsigned int b : 13; int a : 19; } s;
+#endif
     long l;
   } u;
   u.l = l;
-- 
2.26.2

Re: [PATCH v2 0/5] RISC-V big endian support

[Kito Cheng]

Hi Marcus:

Thanks for the quick update, I am testing your V2 patch now, the result seems
really great now, some of fail case seems like not cause by
big-endian patch, I am reviewing and comparing the fail case with the
little-endian build.

> Should I make a PR against riscv-newlib on GitHub, or would you prefer
> some other process for dealing with newlib fixes related to these
> patches?

Could you send to newlib mailing list directly, ideally riscv-newlib
just a buffer
we don't want to hold any patch there as possible.
https://sourceware.org/mailman/listinfo/newlib/




On Sun, Feb 21, 2021 at 8:17 AM Marcus Comstedt <marcus@mc.pp.se> wrote:
>
> This is an update to the patch series for big endian RISC-V support.
>
> Changes since last version:
>
>   * Added documentation of -mbig-endian and -mlittle-endian
>
>   * New patch: Fix soft-fp endianness setting
>
>   * New patch: Fix trampoline generation on big endian
>
>   * New patch: Update the shift-shift-5.c testcase to work correctly
>     on big endian
>
> With these changes, and two fixes to newlib (setting correct floating
> point byteorder, and an update to the handcoded assembler for strcmp),
> I'm now down to
>
>                ========= Summary of gcc testsuite =========
>                             | # of unexpected case / # of unique unexpected case
>                             |          gcc |          g++ |     gfortran |
>      rv64gc/   lp64/ medlow |   14 /     8 |   39 /    10 |      - |
>
> and of these only two failures do not also occur for little endian:
>
> FAIL: gcc.target/riscv/shift-and-1.c scan-assembler-not andi
> FAIL: gcc.target/riscv/shift-and-2.c scan-assembler-not andi
>
> I'm quite puzzled why these two testcases give different results with
> -mbig-endian compared to with -mlittle-endian though, since they only
> deal with register-to-register operations so the memory model should be
> completely irrelevant...
>
>
>   // Marcus
>
>
>

Re: [PATCH v2 0/5] RISC-V big endian support

[Kito Cheng]

Seems like only 3 fail are related to big-endian, you don't need to
worry about other fails.

FAIL: gcc.c-torture/execute/string-opt-5.c
FAIL: gcc.target/riscv/shift-and-1.c scan-assembler-not andi
FAIL: gcc.target/riscv/shift-and-2.c scan-assembler-not andi

On Tue, Feb 23, 2021 at 10:38 AM Kito Cheng <kito.cheng@gmail.com> wrote:
>
> Hi Marcus:
>
> Thanks for the quick update, I am testing your V2 patch now, the result seems
> really great now, some of fail case seems like not cause by
> big-endian patch, I am reviewing and comparing the fail case with the
> little-endian build.
>
> > Should I make a PR against riscv-newlib on GitHub, or would you prefer
> > some other process for dealing with newlib fixes related to these
> > patches?
>
> Could you send to newlib mailing list directly, ideally riscv-newlib
> just a buffer
> we don't want to hold any patch there as possible.
> https://sourceware.org/mailman/listinfo/newlib/
>
>
>
>
> On Sun, Feb 21, 2021 at 8:17 AM Marcus Comstedt <marcus@mc.pp.se> wrote:
> >
> > This is an update to the patch series for big endian RISC-V support.
> >
> > Changes since last version:
> >
> >   * Added documentation of -mbig-endian and -mlittle-endian
> >
> >   * New patch: Fix soft-fp endianness setting
> >
> >   * New patch: Fix trampoline generation on big endian
> >
> >   * New patch: Update the shift-shift-5.c testcase to work correctly
> >     on big endian
> >
> > With these changes, and two fixes to newlib (setting correct floating
> > point byteorder, and an update to the handcoded assembler for strcmp),
> > I'm now down to
> >
> >                ========= Summary of gcc testsuite =========
> >                             | # of unexpected case / # of unique unexpected case
> >                             |          gcc |          g++ |     gfortran |
> >      rv64gc/   lp64/ medlow |   14 /     8 |   39 /    10 |      - |
> >
> > and of these only two failures do not also occur for little endian:
> >
> > FAIL: gcc.target/riscv/shift-and-1.c scan-assembler-not andi
> > FAIL: gcc.target/riscv/shift-and-2.c scan-assembler-not andi
> >
> > I'm quite puzzled why these two testcases give different results with
> > -mbig-endian compared to with -mlittle-endian though, since they only
> > deal with register-to-register operations so the memory model should be
> > completely irrelevant...
> >
> >
> >   // Marcus
> >
> >
> >

Re: [PATCH v2 0/5] RISC-V big endian support

[Marcus Comstedt]

Hi Kito,

Kito Cheng <kito.cheng@gmail.com> writes:

> FAIL: gcc.c-torture/execute/string-opt-5.c
> FAIL: gcc.target/riscv/shift-and-1.c scan-assembler-not andi
> FAIL: gcc.target/riscv/shift-and-2.c scan-assembler-not andi

string-opt-5.c is one of the newlib issues I mentioned (handcoded
assembler for strcmp which assumed LE (it was intended to #error out
on BE, but used "BYTE_ORDER" instead of "__BYTE_ORDER__", so the check
never worked)).  I'll send the fixes later today.

The shift-and tests don't generate incorrect code or anything, but
it's still puzzling why the generated code is different from with
-mlittle-endian.


  // Marcus

Re: [PATCH v2 0/5] RISC-V big endian support

[Kito Cheng]

Hi Marcus:

I just spend some time on those two testcase, I think this those two
testcase could just skip in big-endinan.

> FAIL: gcc.target/riscv/shift-and-1.c scan-assembler-not andi
> FAIL: gcc.target/riscv/shift-and-2.c scan-assembler-not andi

However seems like rv32be has still has some strange fail there,
do you mind take a look for that?

../configure --prefix=$PREFIX --with-arch=rv32gc
--with-multilib-generator=rv32gc-ilp32--


diff --git a/gcc/testsuite/gcc.target/riscv/shift-and-1.c
b/gcc/testsuite/gcc.target/riscv/shift-and-1.c
index d1f3a05db2c..6f4dccc709f 100644
--- a/gcc/testsuite/gcc.target/riscv/shift-and-1.c
+++ b/gcc/testsuite/gcc.target/riscv/shift-and-1.c
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-march=rv32gc -mabi=ilp32 -O" } */
+/* { dg-options "-march=rv32gc -mabi=ilp32 -O -mlittle-endian" } */

/* Test for <optab>si3_mask.  */
int
diff --git a/gcc/testsuite/gcc.target/riscv/shift-and-2.c
b/gcc/testsuite/gcc.target/riscv/shift-and-2.c
index 2c98e50101b..19ce5a60b30 100644
--- a/gcc/testsuite/gcc.target/riscv/shift-and-2.c
+++ b/gcc/testsuite/gcc.target/riscv/shift-and-2.c
@@ -1,5 +1,5 @@
/* { dg-do compile { target { riscv64*-*-* } } } */
-/* { dg-options "-march=rv64gc -mabi=lp64 -O" } */
+/* { dg-options "-march=rv64gc -mabi=lp64 -O -mlittle-endian" } */

/* Test for <optab>si3_mask_1.  */
extern int k;
On Tue, Feb 23, 2021 at 3:23 PM Marcus Comstedt <marcus@mc.pp.se> wrote:
>
>
> Hi Kito,
>
> Kito Cheng <kito.cheng@gmail.com> writes:
>
> > FAIL: gcc.c-torture/execute/string-opt-5.c
> > FAIL: gcc.target/riscv/shift-and-1.c scan-assembler-not andi
> > FAIL: gcc.target/riscv/shift-and-2.c scan-assembler-not andi
>
> string-opt-5.c is one of the newlib issues I mentioned (handcoded
> assembler for strcmp which assumed LE (it was intended to #error out
> on BE, but used "BYTE_ORDER" instead of "__BYTE_ORDER__", so the check
> never worked)).  I'll send the fixes later today.
>
> The shift-and tests don't generate incorrect code or anything, but
> it's still puzzling why the generated code is different from with
> -mlittle-endian.
>
>
>   // Marcus
>
>

Re: [PATCH v2 0/5] RISC-V big endian support

[Marcus Comstedt]

Hi Kito,

Kito Cheng <kito.cheng@gmail.com> writes:

> I just spend some time on those two testcase, I think this those two
> testcase could just skip in big-endinan.

Well, that sounds like a pretty big cop out.  If the software doesn't
behave like we expect it too I feel we should at least have some idea
_why_...


>> FAIL: gcc.target/riscv/shift-and-1.c scan-assembler-not andi
>> FAIL: gcc.target/riscv/shift-and-2.c scan-assembler-not andi
>
> However seems like rv32be has still has some strange fail there,
> do you mind take a look for that?

Do you mean in those two test cases specifically?  Or rv32be in
general?


  // Marcus

Re: [PATCH v2 0/5] RISC-V big endian support

[Marcus Comstedt]

Hi again.

I've found the reason for the shift-and test fails.

riscv.md does a match on

  (subreg:QI (and:SI ...) 0)

Unfortunately, due to the way "subreg" is defined, this needs to be

  (subreg:QI (and:SI ...) 3)

on big endian.  I can fix the failures by duplicating the rule and
making the one with "0" check !BYTES_BIG_ENDIAN and the one with "3"
check BYTES_BIG_ENDIAN.  But that's a bit heavy handed of course.
I'll try to come up with a solution using subreg_lowpart_p instead of
hardcoding "0" or "3".


  // Marcus

Re: [PATCH v2 0/5] RISC-V big endian support

[Marcus Comstedt]

Hi Kito.

I fixed almost all of the rv32be testcase failures simply by taking
endianness into account on the first line of riscv_subword, which is
used for long long handling on 32-bit.

Now, I only have one failing testcase (which does not also fail on
little endian), and it's a doozy.

The test in question is gcc.c-torture/compile/pr35318.c.  The test in
its entirety is

  
  double x = 4, y;
  __asm__ volatile ("# %0,%1,%2,%3" : "=r,r" (x), "=r,r" (y) : "%0,0" (x), "m,r" (8));


(the asm comment in the first argument was added by me to track what
 the actual assignments were.)

When compiled with -mbig-endian, this results in an ICE:

---8<---
/tmp/pr35318.c: In function 'foo':
/tmp/pr35318.c:9:1: error: unrecognizable insn:
    9 | }
      | ^
(insn 12 24 25 2 (parallel [
            (set (reg:DF 11 a1 [orig:74 x ] [74])
                (asm_operands/v:DF ("# %0,%1,%2,%3") ("=r,r") 0 [
                        (reg:SI 12 a2 [orig:74 x+4 ] [74])
                        (mem/c:DF (plus:SI (reg/f:SI 8 s0)
                                (const_int -40 [0xffffffffffffffd8])) [2 %sfp+-24 S8 A64])
                    ]
                     [
                        (asm_input:DF ("%0,0") /tmp/pr35318.c:8)
                        (asm_input:SI ("m,r") /tmp/pr35318.c:8)
                    ]
                     [] /tmp/pr35318.c:8))
            (set (reg:DF 15 a5 [orig:75 y ] [75])
                (asm_operands/v:DF ("# %0,%1,%2,%3") ("=r,r") 1 [
                        (reg:SI 12 a2 [orig:74 x+4 ] [74])
                        (mem/c:DF (plus:SI (reg/f:SI 8 s0)
                                (const_int -40 [0xffffffffffffffd8])) [2 %sfp+-24 S8 A64])
                    ]
                     [
                        (asm_input:DF ("%0,0") /tmp/pr35318.c:8)
                        (asm_input:SI ("m,r") /tmp/pr35318.c:8)
                    ]
                     [] /tmp/pr35318.c:8))
        ]) "/tmp/pr35318.c":8:3 -1
     (nil))
during RTL pass: reload
dump file: /tmp/pr35318b.txt
/tmp/pr35318.c:9:1: internal compiler error: in extract_constrain_insn, at recog.c:2670
0x101bf90b _fatal_insn(char const*, rtx_def const*, char const*, int, char const*)
	../../../riscv-gcc/gcc/rtl-error.c:108
0x101bf953 _fatal_insn_not_found(rtx_def const*, char const*, int, char const*)
	../../../riscv-gcc/gcc/rtl-error.c:116
0x10a1193f extract_constrain_insn(rtx_insn*)
	../../../riscv-gcc/gcc/recog.c:2670
0x1088fc77 check_rtl
	../../../riscv-gcc/gcc/lra.c:2087
0x108971c7 lra(_IO_FILE*)
	../../../riscv-gcc/gcc/lra.c:2505
0x1082fcb7 do_reload
	../../../riscv-gcc/gcc/ira.c:5827
0x1082fcb7 execute
	../../../riscv-gcc/gcc/ira.c:6013
---8<---

This insn looks extremely similar to one that's in the dump-rtl for
little endian:

---8<---
(insn 12 20 21 2 (parallel [
            (set (reg:DF 13 a3 [orig:74 x ] [74])
                (asm_operands/v:DF ("# %0,%1,%2,%3") ("=r,r") 0 [
                        (reg:SI 13 a3 [orig:74 x ] [74])
                        (mem/c:DF (plus:SI (reg/f:SI 8 s0)
                                (const_int -40 [0xffffffffffffffd8])) [2 %sfp+-24 S8 A64])
                    ]
                     [
                        (asm_input:DF ("%0,0") /tmp/pr35318.c:8)
                        (asm_input:SI ("m,r") /tmp/pr35318.c:8)
                    ]
                     [] /tmp/pr35318.c:8))
            (set (reg:DF 15 a5 [orig:75 y ] [75])
                (asm_operands/v:DF ("# %0,%1,%2,%3") ("=r,r") 1 [
                        (reg:SI 13 a3 [orig:74 x ] [74])
                        (mem/c:DF (plus:SI (reg/f:SI 8 s0)
                                (const_int -40 [0xffffffffffffffd8])) [2 %sfp+-24 S8 A64])
                    ]
                     [
                        (asm_input:DF ("%0,0") /tmp/pr35318.c:8)
                        (asm_input:SI ("m,r") /tmp/pr35318.c:8)
                    ]
                     [] /tmp/pr35318.c:8))
        ]) "/tmp/pr35318.c":8:3 -1
     (nil))
---8<---

So I don't know what's "unrecognizable" about it...

I also don't understand the code that is actually generated in the
little-endian case.

The way I read the asm statement, %2 should be a register (same as %0)
containing the (floating point?) value "4", and %3 should be a memory
location (assuming the first alternative is chosen) containing the
value "8".

However, looking at the generated assembler code, it seems that %2 is
a register (a3) which contains the integer value "8" and %3 is a
memory location (-40(s0)) which contains the floating point value
"4.0".  This seems mixed up.

---8<---
foo:
	addi	sp,sp,-48
	sw	s0,44(sp)
	addi	s0,sp,48
	lui	a5,%hi(.LC0)
	fld	fa5,%lo(.LC0)(a5)
	fsd	fa5,-24(s0)
	fld	fa5,-24(s0)
	li	a5,8
	fsd	fa5,-40(s0)
	mv	a3,a5
 #APP
# 8 "/tmp/pr35318.c" 1
	# a3,a5,a3,-40(s0)
# 0 "" 2
 #NO_APP
	sw	a3,-40(s0)
	sw	a4,-36(s0)
	fld	fa5,-40(s0)
	fsd	fa5,-24(s0)
	sw	a5,-32(s0)
	sw	a6,-28(s0)
	nop
	lw	s0,44(sp)
	addi	sp,sp,48
	jr	ra
	.size	foo, .-foo
	.section	.rodata
	.align	3
.LC0:  # little endian double "4.0"
	.word	0
	.word	1074790400
---8<---

Is this code correct, or is there some deeper issue at play here?
(AFAIU the testcase only checks that the compiler doesn't ICE, not
 that the generated code is correct...)

If the code generated for LE is bad, I probably should not try to make
BE generate the same thing.  :-/


  // Marcus

Re: [PATCH v2 0/5] RISC-V big endian support

[Marcus Comstedt]

Hello again Kito.

I've now delved a bit deeper into the failure of the testcase
gcc.c-torture/compile/pr35318.c on big endian RV32.

The point at which big endian diverges from little endian is where
process_alt_operands() is processing the "%0" constraint.  It calls
operands_match_p(), which succeeds on little endian but fails on
big endian.

On little endian, the two rtx:es passed to operands_match_p are
"r79:DF#0" and "r79:DF", while on big endian they are "r79:DF#4" and
"r79:DF".  While the first operand is different, it's actually saying
the same thing: The subreg with the least significant bits (meaning
the second register in the pair on big endian, and the first register
in the pair on little endian, what with two 32-bit integer registers
being allocated to hold a single 64-bit float).

The helper function lra_constraint_offset(), which is used by
operands_match_p, seems to be intended to handle this discrepancy.  It
contains the code

  if (WORDS_BIG_ENDIAN
      && is_a <scalar_int_mode> (mode, &int_mode)
      && GET_MODE_SIZE (int_mode) > UNITS_PER_WORD)
    return hard_regno_nregs (regno, mode) - 1;

However, in this case the rule does not trigger because the mode of
the second operand (which is the one where an adjustment would be
needed) does not have a scalar_int_mode, it has DFmode.  If I relax
this code to also allow scalar_float_mode, then the operands_match_p
call succeeds also on big endian.  There is still an ICE triggered
further down the line though.

I seem to be finding more questions than answers here.  Questions such
as "is it really correct that the first operand to operands_match_p()
has modeSI but the second one has modeDF?", "_should_ the operands
match?", and "why is the least significant half singled out when there
is no computation being perfomed".

Given that the code generated for LE seems incorrect, I still suspect
that there is some deeper issue here not related to endianness (but
possibly related to using integer registers for passing floating point
values to/from asm statements) and that it just happens to not cause
an internal error (only bad code) on LE.

How would you like to proceed?  I don't feel confident that I will
find a definitive solution to this issue anytime soon, but it feels
like such a weird special case (who passes 64-bit floats in 32-bit
integer registers to their asm?) that it might be ok to just ignore
it.  If you agree I'll just repost the patchset with the final fix
added (solves all remaining 32-bit testcases save for this one)...


  // Marcus

Re: [PATCH v2 0/5] RISC-V big endian support

[Kito Cheng]

Hi Marcus:

Thank you for digging this issue out, I would suggest you sent v4
patch which only v3 + riscv_subword fix, and then merge into master
first, and then sent separate patch for that issue, not sure what your
fix, but I guess it might fix some code for IRA/LRA, so I think has a
separate patch would be easy to discussion with other (non-RISC-V)
maintainers.


On Mon, Mar 15, 2021 at 5:42 AM Marcus Comstedt <marcus@mc.pp.se> wrote:
>
>
> Hello again Kito.
>
> I've now delved a bit deeper into the failure of the testcase
> gcc.c-torture/compile/pr35318.c on big endian RV32.
>
> The point at which big endian diverges from little endian is where
> process_alt_operands() is processing the "%0" constraint.  It calls
> operands_match_p(), which succeeds on little endian but fails on
> big endian.
>
> On little endian, the two rtx:es passed to operands_match_p are
> "r79:DF#0" and "r79:DF", while on big endian they are "r79:DF#4" and
> "r79:DF".  While the first operand is different, it's actually saying
> the same thing: The subreg with the least significant bits (meaning
> the second register in the pair on big endian, and the first register
> in the pair on little endian, what with two 32-bit integer registers
> being allocated to hold a single 64-bit float).
>
> The helper function lra_constraint_offset(), which is used by
> operands_match_p, seems to be intended to handle this discrepancy.  It
> contains the code
>
>   if (WORDS_BIG_ENDIAN
>       && is_a <scalar_int_mode> (mode, &int_mode)
>       && GET_MODE_SIZE (int_mode) > UNITS_PER_WORD)
>     return hard_regno_nregs (regno, mode) - 1;
>
> However, in this case the rule does not trigger because the mode of
> the second operand (which is the one where an adjustment would be
> needed) does not have a scalar_int_mode, it has DFmode.  If I relax
> this code to also allow scalar_float_mode, then the operands_match_p
> call succeeds also on big endian.  There is still an ICE triggered
> further down the line though.
>
> I seem to be finding more questions than answers here.  Questions such
> as "is it really correct that the first operand to operands_match_p()
> has modeSI but the second one has modeDF?", "_should_ the operands
> match?", and "why is the least significant half singled out when there
> is no computation being perfomed".
>
> Given that the code generated for LE seems incorrect, I still suspect
> that there is some deeper issue here not related to endianness (but
> possibly related to using integer registers for passing floating point
> values to/from asm statements) and that it just happens to not cause
> an internal error (only bad code) on LE.
>
> How would you like to proceed?  I don't feel confident that I will
> find a definitive solution to this issue anytime soon, but it feels
> like such a weird special case (who passes 64-bit floats in 32-bit
> integer registers to their asm?) that it might be ok to just ignore
> it.  If you agree I'll just repost the patchset with the final fix
> added (solves all remaining 32-bit testcases save for this one)...
>
>
>   // Marcus
>
>

Re: [PATCH v2 0/5] RISC-V big endian support

[Maciej W. Rozycki]

On Sun, 14 Mar 2021, Marcus Comstedt wrote:

> How would you like to proceed?  I don't feel confident that I will
> find a definitive solution to this issue anytime soon, but it feels
> like such a weird special case (who passes 64-bit floats in 32-bit
> integer registers to their asm?) that it might be ok to just ignore
> it.  If you agree I'll just repost the patchset with the final fix
> added (solves all remaining 32-bit testcases save for this one)...

 Soft-float use case?  Also VAX does even for hard float as it does not 
have separate FPRs, but then it is little-endian exclusively too.

 Overall I think this is analogous to `long long' with 32-bit targets, 
though individual psABIs may specify different conventions as to the order 
of the two parts of the FP datum between the registers in such a pair.

  Maciej

Re: [PATCH v2 0/5] RISC-V big endian support

[Jim Wilson]

On Fri, Mar 19, 2021 at 9:22 AM Kito Cheng via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

> On Mon, Mar 15, 2021 at 5:42 AM Marcus Comstedt <marcus@mc.pp.se> wrote:
> > I've now delved a bit deeper into the failure of the testcase
> > gcc.c-torture/compile/pr35318.c on big endian RV32.
>

Looking at this testcase, I think this is triggering undefined behavior for
extended asms.

We have an SImode integer constant 8, a DFmode input/output, a 0
constraint that matches the input to output, and then a % commutative
operator that lets us swap operands, except once we swap operands we are
now trying to force SImode and DFmode values to match via the 0 constraint
which is unreasonable, plus a m constraint that then forces an input to
memory.  It works by accident for little-endian because we reload the +0
word of the double and it is still considered the same operand, and it
fails for big-endian by accident because we reload the +4 word of the
double and now it is considered a different operand.

If I change the "8" to "(double)8" or "8.0" then the testcase works for
both big and little endian, as now we have only DFmode values.

I tried ppc-eabi and ppcle-eabi to see what happens there, and the main
difference is that it chooses the 1 alternative in both cases.  However,
for RISC-V, we choose the 0 alternative with operands swapped.  The reason
for this is that we have a DFmode pseudo that wants an FP reg for a load,
and the same pseudo wants a general reg in the asm, and rv32gc does not
have an instruction to move directly between 64-bit FP regs and 32-bit
general regs, so it gets put in memory as the lowest cost option.  That
then leads to the case that alt 0 with swapped operands has the lowest
cost, except this case is the invalid case that tries to match SImode and
DFmode operands with 0 and m constraints and fails.

To summarize, I think that there are two problems here.
1) The testcase is invalid, and can be fixed by changing the "8" to
"(double)8" or "8.0" to ensure that we have a double constant that matches
the type of the other operands.
2) GCC should be giving an error for an asm like this rather than an ICE.
Note that if I manually swap the operands and remove the % I get
void
foo ()
{
  double x = 4, y;
  __asm__ volatile ("" : "=r" (x), "=r" (y) : "0" (8), "m" (x));
}
which fails with an ICE for big-endian ppc exactly the same as it does for
big-endian RISC-V.  We should be generating an error here rather than an
ICE.

Jim