From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from angie.orcam.me.uk (angie.orcam.me.uk [78.133.224.34]) by sourceware.org (Postfix) with ESMTP id 5E4493858D28 for ; Sat, 6 May 2023 23:11:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5E4493858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=orcam.me.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=orcam.me.uk Received: by angie.orcam.me.uk (Postfix, from userid 500) id 40AA092009C; Sun, 7 May 2023 01:11:01 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by angie.orcam.me.uk (Postfix) with ESMTP id 3339C92009B; Sun, 7 May 2023 00:11:01 +0100 (BST) Date: Sun, 7 May 2023 00:11:01 +0100 (BST) From: "Maciej W. Rozycki" To: david@davidgf.es cc: binutils@sourceware.org Subject: Re: [PATCH 1/3] Add Allegrex CPU as a MIPS2-based CPU In-Reply-To: <20230328230249.274759-2-david@davidgf.es> Message-ID: References: <20230328230249.274759-1-david@davidgf.es> <20230328230249.274759-2-david@davidgf.es> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-1169.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_STATUS,KAM_INFOUSMEBIZ,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, 29 Mar 2023, david@davidgf.es wrote: > From: David Guillen Fandos > > Includes its corresponding gas tests. Please make the change description more elaborate, stating what the Allegrex CPU is (you've referred to the PSP in the cover letter, but that won't be recorded in the repository, so such a reference ought to appear here as well) and naming any relevant available sources of information, such as those used to create this work. We'll need this to maintain this code. > diff --git a/bfd/archures.c b/bfd/archures.c > index ff1ee0f9de..775b2f4889 100644 > --- a/bfd/archures.c > +++ b/bfd/archures.c > @@ -173,6 +173,7 @@ DESCRIPTION > .#define bfd_mach_mips16000 16000 > .#define bfd_mach_mips16 16 > .#define bfd_mach_mips5 5 > +.#define bfd_mach_mips_allegrex 10111431 {* octal 'AL', 31 *} Please add a full stop here and make it two spaces afterwards... > diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h > index b60ff960f0..a7f0930810 100644 > --- a/bfd/bfd-in2.h > +++ b/bfd/bfd-in2.h > @@ -1468,6 +1468,7 @@ enum bfd_architecture > #define bfd_mach_mips16000 16000 > #define bfd_mach_mips16 16 > #define bfd_mach_mips5 5 > +#define bfd_mach_mips_allegrex 10111431 /* octal 'AL', 31 */ ... and regenerate this accordingly. This is as per the GNU Coding Standards: . > diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c > index 35bbd86044..64e64ee142 100644 > --- a/bfd/elfxx-mips.c > +++ b/bfd/elfxx-mips.c > @@ -7070,6 +7070,9 @@ _bfd_elf_mips_mach (flagword flags) > case E_MIPS_MACH_IAMR2: > return bfd_mach_mips_interaptiv_mr2; > > + case E_MIPS_MACH_ALLEGREX: > + return bfd_mach_mips_allegrex; > + > default: > switch (flags & EF_MIPS_ARCH) > { > @@ -12438,6 +12441,10 @@ mips_set_isa_flags (bfd *abfd) > val = E_MIPS_ARCH_64 | E_MIPS_MACH_XLR; > break; > > + case bfd_mach_mips_allegrex: > + val = E_MIPS_ARCH_2 | E_MIPS_MACH_ALLEGREX; > + break; > + These switches were meant to be sorted by the ISA level, though by now people have messed this up already. Let's not continue the trend though, so please put the two new entries below corresponding `bfd_mach_mips4010' ones above. > diff --git a/include/elf/mips.h b/include/elf/mips.h > index e2c3868348..64cab7e6f8 100644 > --- a/include/elf/mips.h > +++ b/include/elf/mips.h > @@ -302,6 +302,7 @@ END_RELOC_NUMBERS (R_MIPS_maxext) > #define E_MIPS_MACH_GS464 0x00A20000 > #define E_MIPS_MACH_GS464E 0x00A30000 > #define E_MIPS_MACH_GS264E 0x00A40000 > +#define E_MIPS_MACH_ALLEGREX 0x00A50000 Do you have any specific need for this particular E_MIPS_MACH value? If not, then can you please leave the subsequent values here for Loongson folks to continue using? There are numerous gaps in this allocation to fill and for a one-off allocation like this I'd suggest using one of them. E.g. 0x00840000. Once we got this in place it'll be a part of the ABI and will have to stay there forever. There is a comment about Cygnus allocations above, but it's of historical value now. > diff --git a/include/opcode/mips.h b/include/opcode/mips.h > index 75d3fc257c..08b4399b0d 100644 > --- a/include/opcode/mips.h > +++ b/include/opcode/mips.h > @@ -1265,6 +1265,8 @@ static const unsigned int mips_isa_table[] = { > #define INSN_XLR 0x00000020 > /* Imagination interAptiv MR2. */ > #define INSN_INTERAPTIV_MR2 0x04000000 > +/* Sony PSP Allegrex instruction. */ > +#define INSN_ALLEGREX 0x08000000 Please use tabs to align this. > diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c > index e911aaa904..ee9362761d 100644 > --- a/gas/config/tc-mips.c > +++ b/gas/config/tc-mips.c > @@ -536,7 +537,7 @@ static int mips_32bitmode = 0; > > /* True, if CPU has support for ldc1 and sdc1. */ > #define CPU_HAS_LDC1_SDC1(CPU) \ > - ((mips_opts.isa != ISA_MIPS1) && ((CPU) != CPU_R5900)) > + ((mips_opts.isa != ISA_MIPS1) && ((CPU) != CPU_R5900) && ((CPU) != CPU_ALLEGREX)) Please wrap the line to stay within 79 columns (74 is the preferred limit where feasible). This also comes from the GNU Coding Standards. > diff --git a/opcodes/mips-dis.c b/opcodes/mips-dis.c > index 859d4e3806..742a47cb69 100644 > --- a/opcodes/mips-dis.c > +++ b/opcodes/mips-dis.c > @@ -706,6 +706,10 @@ const struct mips_arch_choice mips_arch_choices[] = > mips_cp0sel_names_xlr, ARRAY_SIZE (mips_cp0sel_names_xlr), > mips_cp1_names_mips3264, mips_hwr_names_numeric }, > > + { "allegrex", 1, bfd_mach_mips_allegrex, CPU_ALLEGREX, > + ISA_MIPS2, 0, mips_cp0_names_numeric, NULL, 0, > + mips_cp1_names_numeric, mips_hwr_names_numeric }, > + This part of the array is for MIPSr1 and newer architecture processors (yes, someone put "loongson2e"/"loongson2f" entries out of order), so please put the entry for "allegrex" below "r4010" instead, which is where MIPS II entries reside. > diff --git a/opcodes/mips-opc.c b/opcodes/mips-opc.c > index 2a1a6cbb27..cfbcd0b9e1 100644 > --- a/opcodes/mips-opc.c > +++ b/opcodes/mips-opc.c > @@ -3408,11 +3411,11 @@ const struct mips_opcode mips_builtin_opcodes[] = > {"c0", "C", 0x42000000, 0xfe000000, CP, 0, I1, 0, IOCT|IOCTP|IOCT2 }, > {"c1", "C", 0x46000000, 0xfe000000, FP_S, 0, I1, 0, 0 }, > {"c2", "C", 0x4a000000, 0xfe000000, CP, 0, I1, 0, N54|IOCT|IOCTP|IOCT2 }, > -{"c3", "C", 0x4e000000, 0xfe000000, CP, 0, I1, 0, I3_33 }, > +{"c3", "C", 0x4e000000, 0xfe000000, CP, 0, I1, 0, I3_33|AL }, > {"cop0", "C", 0, (int) M_COP0, INSN_MACRO, 0, I1, 0, IOCT|IOCTP|IOCT2 }, > {"cop1", "C", 0, (int) M_COP1, INSN_MACRO, INSN2_M_FP_S, I1, 0, 0 }, > {"cop2", "C", 0, (int) M_COP2, INSN_MACRO, 0, I1, 0, N54|IOCT|IOCTP|IOCT2 }, > -{"cop3", "C", 0, (int) M_COP3, INSN_MACRO, 0, I1, 0, I3_33 }, > +{"cop3", "C", 0, (int) M_COP3, INSN_MACRO, 0, I1, 0, I3_33|AL }, Why do these generic "c3" and "cop3" encodings need to be removed for Allegrex? > diff --git a/gas/testsuite/gas/mips/mips.exp b/gas/testsuite/gas/mips/mips.exp > index e358c9716a..f28ea7e695 100644 > --- a/gas/testsuite/gas/mips/mips.exp > +++ b/gas/testsuite/gas/mips/mips.exp > @@ -536,6 +536,9 @@ mips_arch_create xlr 64 mips64 { oddspreg } \ > mips_arch_create r5900 64 mips3 { gpr_ilocks singlefloat nollsc } \ > { -march=r5900 -mtune=r5900 } { -mmips:5900 } \ > { mipsr5900el-*-* mips64r5900el-*-* } > +mips_arch_create allegrex 32 mips2 { singlefloat oddspreg } \ > + { -march=allegrex -mtune=allegrex } { -mmips:allegrex } \ Please put this below the entry for `r3900' to keep ISA level ordering. Also wrap the line to stay within 79 columns. > + { allegrex-*-* allegrex-*-* } There's no `allegrex-*-*' configuration triplet added by your change (and I'd question it if there was), so please drop this part. > @@ -1858,7 +1861,7 @@ if { [istarget mips*-*-vxworks*] } { > run_dump_test_arches "attr-gnu-4-0" "-mfp32 -32" \ > [mips_arch_list_matching mips1 !mips32r6] > run_dump_test_arches "attr-gnu-4-0" "-mfpxx -32" \ > - [mips_arch_list_matching mips2 !r5900] > + [mips_arch_list_matching mips2 !r5900 !allegrex] Please wrap the line to stay within 79 columns. Likewise throughout the rest of the file. > diff --git a/gas/testsuite/gas/mips/allegrex@isa-override-1.d b/gas/testsuite/gas/mips/allegrex@isa-override-1.d > new file mode 100644 > index 0000000000..5d33ac45d7 > --- /dev/null > +++ b/gas/testsuite/gas/mips/allegrex@isa-override-1.d > @@ -0,0 +1,28 @@ > +#objdump: -dr --prefix-addresses --show-raw-insn > +#name: MIPS ISA override code generation > +#as: -32 > + > +.*: +file format .*mips.* > + > +Disassembly of section \.text: > +[0-9a-f]+ <[^>]*> 8c820000 lw v0,0\(a0\) > +[0-9a-f]+ <[^>]*> 8c830004 lw v1,4\(a0\) > +[0-9a-f]+ <[^>]*> 3c0189ab lui at,0x89ab > +[0-9a-f]+ <[^>]*> 00411025 or v0,v0,at > +[0-9a-f]+ <[^>]*> dc820000 0xdc820000 > +[0-9a-f]+ <[^>]*> 340189ab li at,0x89ab > +[0-9a-f]+ <[^>]*> 00010c38 0x10c38 > +[0-9a-f]+ <[^>]*> 00411025 or v0,v0,at > +[0-9a-f]+ <[^>]*> 3c029000 lui v0,0x9000 > +[0-9a-f]+ <[^>]*> 00021438 0x21438 > +[0-9a-f]+ <[^>]*> 34428000 ori v0,v0,0x8000 > +[0-9a-f]+ <[^>]*> 00021438 0x21438 > +[0-9a-f]+ <[^>]*> 8c820000 lw v0,0\(a0\) > +[0-9a-f]+ <[^>]*> 8c830004 lw v1,4\(a0\) > +[0-9a-f]+ <[^>]*> 3c0189ab lui at,0x89ab > +[0-9a-f]+ <[^>]*> 00411025 or v0,v0,at > +[0-9a-f]+ <[^>]*> 8c820000 lw v0,0\(a0\) > +[0-9a-f]+ <[^>]*> 8c830004 lw v1,4\(a0\) > +[0-9a-f]+ <[^>]*> 3c0189ab lui at,0x89ab > +[0-9a-f]+ <[^>]*> 00411025 or v0,v0,at > + \.\.\. This was produced with outdated binutils and fails with current output: regexp_diff match failure regexp "^[0-9a-f]+ <[^>]*> dc820000 0xdc820000$" line "00000010 dc820000 .word 0xdc820000" regexp_diff match failure regexp "^[0-9a-f]+ <[^>]*> 00010c38 0x10c38$" line "00000018 00010c38 .word 0x10c38" regexp_diff match failure regexp "^[0-9a-f]+ <[^>]*> 00021438 0x21438$" line "00000024 00021438 .word 0x21438" regexp_diff match failure regexp "^[0-9a-f]+ <[^>]*> 00021438 0x21438$" line "0000002c 00021438 .word 0x21438" FAIL: MIPS ISA override code generation (allegrex) Please update the dump accordingly, and always rebase and verify against the current upstream trunk before submitting. Have you actually been able to run regression testing of your change? If so, what target configuration did you use? > diff --git a/gas/testsuite/gas/mips/allegrex@isa-override-1.s b/gas/testsuite/gas/mips/allegrex@isa-override-1.s > new file mode 100644 > index 0000000000..02352f8554 > --- /dev/null > +++ b/gas/testsuite/gas/mips/allegrex@isa-override-1.s > @@ -0,0 +1,23 @@ > + .text > + .globl foo > + .ent foo > +foo: > + ld $2, 0($4) > + or $2, 0x89ab0000 > + .set push > + .set mips3 > + ld $2, 0($4) > + or $2, 0x89ab0000 > + dli $2, 0x9000000080000000 > + .set mips0 > + ld $2, 0($4) > + or $2, 0x89ab0000 > + .set mips3 > + .set pop > + ld $2, 0($4) > + or $2, 0x89ab0000 > + .end foo > + > +# Force some (non-delay-slot) zero bytes, to make 'objdump' print ... > + .align 4, 0 > + .space 16 This is the same as gas/testsuite/gas/mips/r5900@isa-override-1.s. Please remove then and use: #source: r5900@isa-override-1.s in gas/testsuite/gas/mips/allegrex@isa-override-1.d instead. Maciej