Re: [PATCH] MIPS/opcodes: Fix alias annotation for negate instructions

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Mon, 7 Mar 2022, Sagar Patel wrote:

> On both regular MIPS and microMIPS 32-bit, the NEG and NEGU assembly
> instructions are idioms for SUB and SUBU respectively with the `rs'
> operand equal to $0.

 Thank you.  Your change has passed regression testing against my usual 
set of 87 MIPS targets and overall it looks good to me except for a couple 
of minor nits, as below.

> diff --git a/binutils/ChangeLog b/binutils/ChangeLog
> index a355e18de74..1f134dbe87f 100644
> --- a/binutils/ChangeLog
> +++ b/binutils/ChangeLog
> @@ -1,3 +1,14 @@
> +2022-03-07  Sagar Patel  <sagarmp@cs.unc.edu>
> +
> +	* testsuite/binutils-all/mips/micromips-neg-alias.d: New test.
> +	* testsuite/binutils-all/mips/micromips-neg-alias.s: New test
> +	source.
> +	* testsuite/binutils-all/mips/micromips-neg-noalias.d: New test.
> +	* testsuite/binutils-all/mips/mips-neg-alias.d: New test.
> +	* testsuite/binutils-all/mips/mips-neg-alias.s: New test source.
> +	* testsuite/binutils-all/mips/mips-neg-noalias.d: New test.
> +	* testsuite/binutils-all/mips/mips.exp: Run the new tests.
> +

 Please do not include updates to ChangeLog files with the diff as they 
will almost surely cause a conflict right away.  Instead please just put 
entries intended for ChangeLog files within the change description, as you 
correctly did, and then the committer will produce actual ChangeLog file 
entries from that text.  It could be that eventually we will switch to 
doing this automatically just as GCC does or stop using ChangeLog entries 
altogether like GDB, which will make backports so much easier.

> diff --git a/binutils/testsuite/binutils-all/mips/micromips-neg-alias.s b/binutils/testsuite/binutils-all/mips/micromips-neg-alias.s
> new file mode 100644
> index 00000000000..f5122d3b2dc
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/mips/micromips-neg-alias.s
> @@ -0,0 +1,14 @@
> +	.text
> +	.set	mips32r3
> +	.set	noat
> +	.set	noreorder

 This `.set noreorder' is not needed here (I only put it in the other test 
case to avoid clutter from branch delay slots).

 FAOD the `.set mips32r3' pseudo-op is required so that configurations 
that default to MIPSr6 still assemble this source.

> diff --git a/binutils/testsuite/binutils-all/mips/mips-neg-alias.s b/binutils/testsuite/binutils-all/mips/mips-neg-alias.s
> new file mode 100644
> index 00000000000..bb7b3c5ac3a
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/mips/mips-neg-alias.s
> @@ -0,0 +1,13 @@
> +	.text
> +	.set	noat
> +	.set	noreorder
> +	.set	mips2

 Likewise, and also this `.set mips2' is not needed here (I put it in the
other test case so that branch likely instructions, which require the ISA 
to be between MIPS II and MIPSr6, assemble correctly).

 This might be overly pedantic, but with any test case I suggest putting 
the absolute minimum there so as not to make someone in the future wonder 
what the specific requirement was for one option or another (possibly I 
shouldn't have used `.set noat' and $1 either, but let's leave it as it 
is).

 It looks perfect otherwise and once your paperwork has been sorted I will 
push either your change as posted with amendments made for the two pieces 
above or any v2 if you prefer to make one yourself.

 Thank you for your submission.

  Maciej