Re: [PATCH v2] sim: riscv: Fix some compatibility issues with gcc

[Andrew Burgess <aburgess@redhat.com>]

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> This makes the riscv simulator able to execute a simple
> "hello world" program when gcc is configured
> with:
>
> .../gcc-trunk/configure --target=riscv-unknown-elf
>
> The first problem is that gcc generates rv32
> code by default in this configuration, while
> riscv64-unknown-elf generates rv64 code by default.
>
> So change the riscv/acinclude.m4 to use the same
> logic here.

I'm sorry, but this change is not OK.  At least, I'll need more
convincing before this could be merged.

Previously, the riscv sim would support ( in theory ) both RV32 and
RV64, and would pick the correct word size based on the ELF
architecture.

What you're finding is that the riscv sim is (apparently) pretty buggy.

With just the sim-main.c fix and the model_list.def change I can see the
simulator trying to run the RV32 code.  But, the problem I see is
confusion about whether registers should be sign extended or not.

One example of where this goes wrong is this.  Consider the instruction:

  c.li a1,-1

this will result in the value 0xffffffffffffffff being written into the
register, even for RV32 code.

But as this is 32-bit code, I also see the code build the constant
0xffffffff by shifting and or-ing things together.  The code then
compares the register containing 0xffffffffffffffff with the register
containing 0xffffffff together, and expects them to be the same.

Obviously they are not the same, so the code takes an incorrect branch,
and everything goes wrong...

If we force the build to be RV32 only then the register size will be
32-bits and the above problem is "solved".  But it's not really, we're
just masking the issue.

So I think we need to solve the register size issue first.

>
> And the second issue is that gcc does by default
> generate instructions in INSN_CLASS_C, so move
> the M(GC) to top of list, in riscv/model_list.def.

I haven't looked at this part of the change yet.  But just going by my
gut, this doesn't feel right.  I would have expected there to be some
logic "somewhere" that checks down the model list and looks for a mode
with the required extensions.

Just moving the one we want/need to the top feels like we're just
changing the default.  Is the "pick the correct model" code broken?
Completely missing?

>
> Then there was apparently a confusion which cpu
> model uses JAL and which ADDIW.  Fixed that in
> execute_c, case MATCH_C_JAL | MATCH_C_ADDIW.

You might consider breaking this change out, as this third part could go
in on its own I think.

Thanks,
Andrew


>
> With these changes a simple c-prgram can be executed,
> however there is still work to do, since when the
> program does floating point operations, gcc starts to
> generate hardware floating point instructions, with no
> obvious opt-out option.
>
> Note the gcc test suite can be used to test the
> simulator in this way:
>
> make check-gcc RUNTESTFLAGS="--target_board=multi-sim SIM=riscv-unknown-elf-run"
>
> Now many tests are passed, except those which use
> floating point instructions.
>
> To work around the not supported float instructions the
> following gcc configuration can be used, which makes
> most of the gcc test cases successfully executed:
>
> .../gcc-trunk/configure --prefix=... --target=riscv-unknown-elf
>  --disable-multilib --with-arch=rv32imac --with-abi=ilp32
>
> Note: binutils are installed at prefix path and newlib/libgloss in-tree.
>
> Fixes 3224e32fb84f ("sim: riscv: Add support for compressed integer instructions")
> ---
>  sim/configure            | 6 +++---
>  sim/riscv/acinclude.m4   | 4 ++--
>  sim/riscv/model_list.def | 2 +-
>  sim/riscv/sim-main.c     | 4 ++--
>  4 files changed, 8 insertions(+), 8 deletions(-)
>
> v2: updated the commit message, with some hints
> how to compile a compatible gcc toolchain.
>
> diff --git a/sim/configure b/sim/configure
> index 1ebef377973..fdc0a86d524 100755
> --- a/sim/configure
> +++ b/sim/configure
> @@ -17479,10 +17479,10 @@ $as_echo "$sim_ppc_xor_endian" >&6; }
>  
>  { $as_echo "$as_me:${as_lineno-$LINENO}: checking riscv bitsize" >&5
>  $as_echo_n "checking riscv bitsize... " >&6; }
> -SIM_RISCV_BITSIZE=64
> +SIM_RISCV_BITSIZE=32
>  case $target in #(
> -  riscv32*) :
> -    SIM_RISCV_BITSIZE=32 ;; #(
> +  riscv64*) :
> +    SIM_RISCV_BITSIZE=64 ;; #(
>    *) :
>       ;;
>  esac
> diff --git a/sim/riscv/acinclude.m4 b/sim/riscv/acinclude.m4
> index 0c6290c9c08..e9953c1017d 100644
> --- a/sim/riscv/acinclude.m4
> +++ b/sim/riscv/acinclude.m4
> @@ -15,8 +15,8 @@ dnl along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  dnl
>  dnl NB: This file is included in sim/configure, so keep settings namespaced.
>  AC_MSG_CHECKING([riscv bitsize])
> -SIM_RISCV_BITSIZE=64
> +SIM_RISCV_BITSIZE=32
>  AS_CASE([$target],
> -	[riscv32*], [SIM_RISCV_BITSIZE=32])
> +	[riscv64*], [SIM_RISCV_BITSIZE=64])
>  AC_MSG_RESULT([$SIM_RISCV_BITSIZE])
>  AC_SUBST(SIM_RISCV_BITSIZE)
> diff --git a/sim/riscv/model_list.def b/sim/riscv/model_list.def
> index b83557e5539..df9ec897126 100644
> --- a/sim/riscv/model_list.def
> +++ b/sim/riscv/model_list.def
> @@ -1,9 +1,9 @@
> +M(GC)
>  M(G)
>  M(I)
>  M(IM)
>  M(IMA)
>  M(IA)
> -M(GC)
>  M(IC)
>  M(IMC)
>  M(IMAC)
> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
> index adff99921c6..9c0d070aa60 100644
> --- a/sim/riscv/sim-main.c
> +++ b/sim/riscv/sim-main.c
> @@ -1018,7 +1018,7 @@ execute_c (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>      case MATCH_C_JAL | MATCH_C_ADDIW:
>        /* JAL and ADDIW have the same mask but are only available on RV64 or
>  	 RV32 respectively.  */
> -      if (RISCV_XLEN (cpu) == 64)
> +      if (RISCV_XLEN (cpu) == 32)
>  	{
>  	  imm = EXTRACT_CJTYPE_IMM (iw);
>  	  TRACE_INSN (cpu, "c.jal %" PRIxTW,
> @@ -1027,7 +1027,7 @@ execute_c (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>  	  pc = riscv_cpu->pc + imm;
>  	  TRACE_BRANCH (cpu, "to %#" PRIxTW, pc);
>  	}
> -      else if (RISCV_XLEN (cpu) == 32)
> +      else if (RISCV_XLEN (cpu) == 64)
>  	{
>  	  imm = EXTRACT_CITYPE_IMM (iw);
>  	  TRACE_INSN (cpu, "c.addiw %s, %s, %#" PRIxTW ";  // %s += %#" PRIxTW,
> -- 
> 2.39.2