[PATCH] MIPS: Fix encoding of sigrie instruction

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] MIPS: Fix encoding of sigrie instruction

Henry Wong
Hi,

The instruction encoding for the MIPS r6 sigrie instruction seems to be
incorrect. It's currently 0x4170xxxx (which overlaps with ei, di, evp,
and dvp), but should be 0x0417xxxx (See ISA reference,
https://www.mips.com/?do-download=the-mips32-instruction-set-v6-06, page
385 of the PDF)

The attached patch changes the MIPS opcode table and the tests that are
affected. I'm not entirely confident there aren't more changes elsewhere
that need to be made, but gas generates the changed opcode and objdump
also disassembles the changed opcode correctly.


Changes:

    opcodes/mips-opc.c: Update MIPS opcode table
    gas/testsuite/gas/mips/
        * r6.d
        * r6-n32.d
        * r6-n64.d    Update tests that use sigrie


0001-MIPS-Fix-encoding-for-MIPSr6-sigrie-instruction.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[committed] MIPS: Fix encoding of sigrie instruction

Maciej W. Rozycki-2
From: Henry Wong <[hidden email]>

The instruction encoding for the MIPS r6 sigrie instruction seems to be
incorrect.  It's currently 0x4170xxxx (which overlaps with ei, di, evp,
and dvp), but should be 0x0417xxxx.  See ISA reference[1][2].

References:

[1] "MIPS Architecture for Programmers Volume II-A: The MIPS32
    Instruction Set Manual", Imagination Technologies, Inc., Document
    Number: MD00086, Revision 6.06, December 15, 2016, Table A.4 "MIPS32
    REGIMM Encoding of rt Field", p. 452

[2] "MIPS Architecture For Programmers Volume II-A: The MIPS64
    Instruction Set Reference Manual", Imagination Technologies, Inc.,
    Document Number: MD00087, Revision 6.06, December 15, 2016, Table
    A.4 "MIPS64 REGIMM Encoding of rt Field", p. 581

        opcodes/
        * mips-opc.c (mips_builtin_opcodes): Correct "sigrie" encoding.

        gas/
        * testsuite/gas/mips/r6.d: Update for "sigrie" encoding fix.
        * testsuite/gas/mips/r6-n32.d: Likewise.
        * testsuite/gas/mips/r6-n64.d: Likewise.
---
Hi Henry,

 Thank you for your contribution, and for addressing the problem you have
found.

 Your change looks good except for for minor issues with the patch
description, which is also missing in the first place from the otherwise
correctly formed GIT-style patch attached.  I'll address these issues
below.

 First of all however, please indicate what your legal status is with the
Free Software Foundation with regard to change submissions to binutils.  
Have you got a copyright assignment or an alternative arrangement in
place?

 I am asking because together with your previous contribution, commit
487958d1e995 ("Fix segfault processing nios2 pseudo-instructions with too
few arguments."), corresponding to your originally submission archived
here: <https://sourceware.org/ml/binutils/2017-10/msg00012.html>, all your
code submitted so far to binutils looks to me legally significant, and as
such requires a legal procedure to follow before we can accept further
changes from you.

 If you do not have an arrangement with the Free Software Foundation, then
I'll let you know how to proceed.

 I have decided to commit your change due to its small size and triviality
(i.e. there's no other way to fix this bug), however for any further
submissions you'll have to have an arrangement with the Free Software
Foundation before a change can be accepted.

> The instruction encoding for the MIPS r6 sigrie instruction seems to be
> incorrect. It's currently 0x4170xxxx (which overlaps with ei, di, evp,
> and dvp), but should be 0x0417xxxx (See ISA reference,
> https://www.mips.com/?do-download=the-mips32-instruction-set-v6-06, page
> 385 of the PDF)

 This explanation is good as the patch description, so I have used it,
with small adjustments, observing that it is instruction bit encoding
tables rather than bit patterns shown in individual instruction
descriptions that are normative (we had discrepancies in the past), and
also referring to architecture specifications by their titles to avoid a
web link which may turn out volatile in the long term (which happened
more than once).

 Please avoid submitting patches with only change heading included, except
for obvious changes, such as documentation typos, formatting fixes, etc.

> The attached patch changes the MIPS opcode table and the tests that are
> affected. I'm not entirely confident there aren't more changes elsewhere
> that need to be made, but gas generates the changed opcode and objdump
> also disassembles the changed opcode correctly.

 Your changes to the test suite indicate that you have actually regression
tested your change before submitting it.  This is a very good practice
indeed, but in the future please tell us that you actually did it,
indicating your target(s) chosen, e.g. `mips-elf', etc.

> Changes:
>
>     opcodes/mips-opc.c: Update MIPS opcode table
>     gas/testsuite/gas/mips/
>         * r6.d
>         * r6-n32.d
>         * r6-n64.d    Update tests that use sigrie

 ChangeLog entries submitted need to be included at the end of the change
description, using a fixed format.  In particular you need to include full
relative paths of all files affected, with the top subdirectory removed
(i.e. without `opcodes/', `gas/' at the beginning).  You also need to name
the entity updated, in parentheses, where applicable.  See the change
description above, existing ChangeLog files and:

<https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs>

for a full explanation.

 I have committed your change now, in the form recorded here.

 Thank you,

  Maciej
---
 gas/testsuite/gas/mips/r6-n32.d |    4 ++--
 gas/testsuite/gas/mips/r6-n64.d |    4 ++--
 gas/testsuite/gas/mips/r6.d     |    4 ++--
 opcodes/mips-opc.c              |    2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/gas/testsuite/gas/mips/r6-n32.d b/gas/testsuite/gas/mips/r6-n32.d
index fb55086..c9efa1d 100644
--- a/gas/testsuite/gas/mips/r6-n32.d
+++ b/gas/testsuite/gas/mips/r6-n32.d
@@ -497,6 +497,6 @@ Disassembly of section .text:
 0+0598 <[^>]*> 41600024 dvp
 0+059c <[^>]*> 41620004 evp v0
 0+05a0 <[^>]*> 41620024 dvp v0
-0+05a4 <[^>]*> 41700000 sigrie 0x0
-0+05a8 <[^>]*> 4170ffff sigrie 0xffff
+0+05a4 <[^>]*> 04170000 sigrie 0x0
+0+05a8 <[^>]*> 0417ffff sigrie 0xffff
  \.\.\.
diff --git a/gas/testsuite/gas/mips/r6-n64.d b/gas/testsuite/gas/mips/r6-n64.d
index fd4da21..fa0e86b 100644
--- a/gas/testsuite/gas/mips/r6-n64.d
+++ b/gas/testsuite/gas/mips/r6-n64.d
@@ -753,6 +753,6 @@ Disassembly of section .text:
 0+0598 <[^>]*> 41600024 dvp
 0+059c <[^>]*> 41620004 evp v0
 0+05a0 <[^>]*> 41620024 dvp v0
-0+05a4 <[^>]*> 41700000 sigrie 0x0
-0+05a8 <[^>]*> 4170ffff sigrie 0xffff
+0+05a4 <[^>]*> 04170000 sigrie 0x0
+0+05a8 <[^>]*> 0417ffff sigrie 0xffff
  \.\.\.
diff --git a/gas/testsuite/gas/mips/r6.d b/gas/testsuite/gas/mips/r6.d
index 8588e92..9faa478 100644
--- a/gas/testsuite/gas/mips/r6.d
+++ b/gas/testsuite/gas/mips/r6.d
@@ -496,6 +496,6 @@ Disassembly of section .text:
 0+0598 <[^>]*> 41600024 dvp
 0+059c <[^>]*> 41620004 evp v0
 0+05a0 <[^>]*> 41620024 dvp v0
-0+05a4 <[^>]*> 41700000 sigrie 0x0
-0+05a8 <[^>]*> 4170ffff sigrie 0xffff
+0+05a4 <[^>]*> 04170000 sigrie 0x0
+0+05a8 <[^>]*> 0417ffff sigrie 0xffff
  \.\.\.
diff --git a/opcodes/mips-opc.c b/opcodes/mips-opc.c
index 180d613..b0c6195 100644
--- a/opcodes/mips-opc.c
+++ b/opcodes/mips-opc.c
@@ -1867,7 +1867,7 @@ const struct mips_opcode mips_builtin_opcodes[] =
 {"shfl.repa.qh", "X,Y,Z", 0x7b20001f, 0xffe0003f, WR_1|RD_2|RD_3|FP_D, 0, 0, MX, 0 },
 {"shfl.repb.qh", "X,Y,Z", 0x7ba0001f, 0xffe0003f, WR_1|RD_2|RD_3|FP_D, 0, 0, MX, 0 },
 {"shfl.upsl.ob", "X,Y,Z", 0x78c0001f, 0xffe0003f, WR_1|RD_2|RD_3|FP_D, 0, SB1, MX, 0 },
-{"sigrie", "u", 0x41700000, 0xffff0000, TRAP, 0, I37, 0, 0 },
+{"sigrie", "u", 0x04170000, 0xffff0000, TRAP, 0, I37, 0, 0 },
 {"sle", "d,v,t", 0,    (int) M_SLE, INSN_MACRO, 0, I1, 0, 0 },
 {"sle", "d,v,I", 0,    (int) M_SLE_I, INSN_MACRO, 0, I1, 0, 0 },
 {"sle", "S,T", 0x46a0003e, 0xffe007ff, RD_1|RD_2|WR_CC|FP_D, 0, IL2E, 0, 0 },