Re: [PATCH 4/5] sim: mips: Add simulator support for mips32r6/mips64r6

[Mike Frysinger <vapier@gentoo.org>]

On 20 May 2021 13:15, Faraz Shahbazker wrote:
> 2021-05-19  Ali Lown  <ali.lown@imgtec.com>
> 	    Andrew Bennett  <andrew.bennett@imgtec.com>
> 	    Dragan Mladjenovic  <dragan.mladjenovic@rt-rk.com>
> 	    Faraz Shahbazker  <fshahbazker@wavecomp.com>

i assume everyone has FSF papers in place ?

i can't provide too much substantive review on the ISA stuff here.

> --- a/sim/mips/cp1.c
> +++ b/sim/mips/cp1.c
>
> +unsigned64
> +fp_classify(sim_cpu *cpu,
>
> +int
> +fp_rint(sim_cpu *cpu,
>
> +unsigned64
> +fp_min(sim_cpu *cpu,
>
> +unsigned64
> +fp_max(sim_cpu *cpu,
>
> +unsigned64
> +fp_mina(sim_cpu *cpu,
>
> +unsigned64
> +fp_maxa(sim_cpu *cpu,
>
>  unsigned64
>  fp_madd(sim_cpu *cpu,
>
>  unsigned64
>  fp_msub(sim_cpu *cpu,

missing space before the (

should these (and others in this file) be static ?  they aren't namespaced.

> +unsigned64
> +fp_fmsub (sim_cpu* cpu,

shoul be "sim_cpu *cpu".  comes up a few times in here.

> --- a/sim/mips/interp.c
> +++ b/sim/mips/interp.c
>  
>  /* Load a word from memory.  */
> +#define MIPSR6_P(abfd) \
> +  ((elf_elfheader (abfd)->e_flags & EF_MIPS_ARCH) == E_MIPS_ARCH_32R6 \
> +    || (elf_elfheader (abfd)->e_flags & EF_MIPS_ARCH) == E_MIPS_ARCH_64R6)

define should be before the comment

> --- a/sim/mips/mips.igen
> +++ b/sim/mips/mips.igen
>
> +:function:::void:signal_if_cti:instruction_word instr
> +{
> +  unsigned32 maj = (instr & 0xfc000000) >> 26;
> +  unsigned32 special = instr & 0x3f;
> +  if((maj & 0x3e) == 0x06 /* Branch/Jump */

space after "if"

> --- a/sim/mips/sim-main.h
> +++ b/sim/mips/sim-main.h
>
> +static const int sim_fpu_class_mips_mapping[] = {

shouldn't put these in the header

> diff --git a/sim/testsuite/mips/utils-r6.s b/sim/testsuite/mips/utils-r6.s
> new file mode 100644
> index 00000000000..e69de29bb2d

it's an empty file ?
-mike