[PATCH] MIPS: Correct opcode for some IL3A instructions

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

[PATCH] MIPS: Correct opcode for some IL3A instructions

Jiaxun Yang
Correct instrotions' opcode according to Loongson 3A2000's user manual.
Ref(in Chinese): http://www.loongson.cn/uploadfile/cpu/3A2000/Loongson3A2000_user2.pdf

ChangeLog:

opcodes/
        * mips-opc.c
        (mips_opcodes): Fix opcodes gslwlec1,
         gslwgtc1, gsldlec1, gsldgtc1.
---
 opcodes/mips-opc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/opcodes/mips-opc.c b/opcodes/mips-opc.c
index 180d613c93..248aab5124 100644
--- a/opcodes/mips-opc.c
+++ b/opcodes/mips-opc.c
@@ -470,10 +470,10 @@ const struct mips_opcode mips_builtin_opcodes[] =
 {"gsswgt", "t,b,d", 0xe8000015, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
 {"gssdle", "t,b,d", 0xe8000016, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
 {"gssdgt", "t,b,d", 0xe8000017, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
-{"gslwlec1", "T,b,d", 0xc8000018, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
-{"gslwgtc1", "T,b,d", 0xc8000019, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
-{"gsldlec1", "T,b,d", 0xc800001a, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
-{"gsldgtc1", "T,b,d", 0xc800001b, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
+{"gslwlec1", "T,b,d", 0xc800001c, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
+{"gslwgtc1", "T,b,d", 0xc800001d, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
+{"gsldlec1", "T,b,d", 0xc800001e, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
+{"gsldgtc1", "T,b,d", 0xc800001f, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
 {"gsswlec1", "T,b,d", 0xe800001c, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
 {"gsswgtc1", "T,b,d", 0xe800001d, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
 {"gssdlec1", "T,b,d", 0xe800001e, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
--
2.16.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] MIPS: Correct opcode for some IL3A instructions

Maciej W. Rozycki-2
Hi Jiaxun,

 Thank you for your contribution.

 I will be happy to accept your change, however I need a better
documentation reference as the justification.  There are a couple of minor
issues as well.  See below for details.

> Correct instrotions' opcode according to Loongson 3A2000's user manual.
> Ref(in Chinese): http://www.loongson.cn/uploadfile/cpu/3A2000/Loongson3A2000_user2.pdf

 Are you sure it is the right reference though?  I looked through the
manual and I cannot find encodings for these instructions shown anywhere.  
Granted, I might well be missing something, given my regrettable lack of
skills in Chinese, so I will appreciate a specific page pointer if indeed
the required information is there and its only me unable to find it.

 In future submissions please wrap the description so as to make lines fit
in 74 columns (and I recommend 72 columns, due to how indentation works
with `git show', etc.).  Also two spaces are required after full stops.  
For a complete explanation of the rules see:

<https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Formatting_Your_Code>

and:

<https://www.gnu.org/prep/standards/standards.html#Formatting>.

> ChangeLog:
>
> opcodes/
> * mips-opc.c
> (mips_opcodes): Fix opcodes gslwlec1,
> gslwgtc1, gsldlec1, gsldgtc1.

 I am glad that you have included a ChangeLog entry, which is required.  
Your formatting is mostly good, but it has been unnecessarily wrapped.  
The 74 (or 72) column rule applies here as well, so:

        * mips-opc.c (mips_opcodes): Fix opcodes gslwlec1, gslwgtc1,
        gsldlec1, gsldgtc1.

would be my recommendation (a single leading tab and no further
indentation for subsequent lines).

> diff --git a/opcodes/mips-opc.c b/opcodes/mips-opc.c
> index 180d613c93..248aab5124 100644
> --- a/opcodes/mips-opc.c
> +++ b/opcodes/mips-opc.c
> @@ -470,10 +470,10 @@ const struct mips_opcode mips_builtin_opcodes[] =
>  {"gsswgt", "t,b,d", 0xe8000015, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
>  {"gssdle", "t,b,d", 0xe8000016, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
>  {"gssdgt", "t,b,d", 0xe8000017, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
> -{"gslwlec1", "T,b,d", 0xc8000018, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
> -{"gslwgtc1", "T,b,d", 0xc8000019, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
> -{"gsldlec1", "T,b,d", 0xc800001a, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
> -{"gsldgtc1", "T,b,d", 0xc800001b, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
> +{"gslwlec1", "T,b,d", 0xc800001c, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
> +{"gslwgtc1", "T,b,d", 0xc800001d, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
> +{"gsldlec1", "T,b,d", 0xc800001e, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
> +{"gsldgtc1", "T,b,d", 0xc800001f, 0xfc0007ff, WR_1|RD_2|RD_3|LM, 0, IL3A, 0, 0 },
>  {"gsswlec1", "T,b,d", 0xe800001c, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
>  {"gsswgtc1", "T,b,d", 0xe800001d, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },
>  {"gssdlec1", "T,b,d", 0xe800001e, 0xfc0007ff, RD_1|RD_2|RD_3|SM, 0, IL3A, 0, 0 },

 This is fine by itself, as long as you are able to provide me with a
backing documentation reference I requested above.

 However this causes a testsuite regression:

FAIL: Loongson-3A tests

which also needs to be corrected, in gas/testsuite/gas/mips/loongson-3a.d.

 Please always regression-test any changes you propose -- it is as easy as
running `make check' after building binutils and watching out for changes
in the output produced compared to the output obtained from an unpatched
build.  Detailed logs will be provided in: binutils/binutils.log,
gas/testsuite/gas.log and ld/ld.log, for the binutils, GAS and LD subsets
of tests respectively.  Substitute .sum for .log as the file suffix for
test summaries.  Let me know if you need further assistance with
regression testing.

 Since this is one of your first submissions to binutils, I will be happy
to correct all these issues for you to give you guidance examples for the
future, however before I go ahead and commit your change I need that
documentation reference, so please try to locate one and come back to me.  

 I'll be looking into your other submission shortly.

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] MIPS: Correct opcode for some IL3A instructions

Jiaxun Yang
In reply to this post by Jiaxun Yang
Hi Maciej

> I will be happy to accept your change, however I need a better
> documentation reference as the justification.  There are a couple of
minor
> issues as well.  See below for details.

> > Correct instrotions' opcode according to Loongson 3A2000's user
manual.
> > Ref(in Chinese): http://www.loongson.cn/uploadfile/cpu/3A2000/Loong
son3A2000_user2.pdf

>  Are you sure it is the right reference though?  I looked through
the
> manual and I cannot find encodings for these instructions shown
anywhere.  
> Granted, I might well be missing something, given my regrettable lack
of
> skills in Chinese, so I will appreciate a specific page pointer if
indeed

Well, in this document, they didn't exactly mention the opcode, and I
infered the opcodes by the sequences of these instructions appeared in
this document.... According my test, they are right.

After checked all loongson's publictions, I realized no document from
Loongson can clearly proof this.

Will acks from Loongson and Lemote staffs be enough to back this
change?

Sorry for the supper long delay because I missed this E-mail.

Thanks.

> the required information is there and its only me unable to find it.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] MIPS: Correct opcode for some IL3A instructions

Maciej W. Rozycki-2
Hi Jiaxun,

> >  Are you sure it is the right reference though?  I looked through
> the
> > manual and I cannot find encodings for these instructions shown
> anywhere.  
> > Granted, I might well be missing something, given my regrettable lack
> of
> > skills in Chinese, so I will appreciate a specific page pointer if
> indeed
>
> Well, in this document, they didn't exactly mention the opcode, and I
> infered the opcodes by the sequences of these instructions appeared in
> this document.... According my test, they are right.
>
> After checked all loongson's publictions, I realized no document from
> Loongson can clearly proof this.
>
> Will acks from Loongson and Lemote staffs be enough to back this
> change?

 For long-term maintenance reasons we require published backing
documentation for things like instruction encodings, so that for example
if we spot a bug in code somewhere or do a code rewrite, then we can
make a fix or go ahead with changes respectively without having to guess
or chase people who may not be available anymore.

 You don't need to document the semantics of individual instructions.  
Just listing the instruction formats used (i.e. where the individual
operands go in the instruction word) and opcode encodings will be
enough.  That is you do not need to disclose anything beyond what is
already there in libopcodes.

 I don't know why the original submission for the affected instructions
has been accepted in the first place; we've had this rule for backing
documentation since forever.

 I hope this clears the situation.  If you have further questions, then
please let me know.

  Maciej