[PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register

[PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register

[Maciej W. Rozycki]

Hi,

 Here's another one:

$ cat ld-base.s
	ld	$4, ($4)
$ mips-linux-as -o ld-base.o ld-base.s
$ mips-linux-objdump -d ld-base.o

ld-base.o:     file format elf32-tradbigmips


Disassembly of section .text:

00000000 <.text>:
   0:	8c840000 	lw	a0,0(a0)
   4:	00000000 	nop
   8:	8c850004 	lw	a1,4(a0)

Ouch!

 Fixed thus and regression tested for mips-linux, mips64-linux, 
mipstx39-elf, mipsisa64-elf and mips-ecoff targets and their little-endian 
counterparts.  The new code produced looks like this:

$ mips-linux-objdump -d ld-base.o

ld-base.o:     file format elf32-tradbigmips


Disassembly of section .text:

00000000 <.text>:
   0:	00800821 	move	at,a0
   4:	8c240000 	lw	a0,0(at)
   8:	8c250004 	lw	a1,4(at)

2010-10-17  Maciej W. Rozycki  <macro@linux-mips.org>

	gas/
	* config/tc-mips.c (macro)[M_LD_OB]: Use a temporary register
	when the first load of the pair would overwrite the base
	register needed for the other one.

 OK?  And thanks for your reviews so far -- I'll try to proceed with 
commits ASAP.

  Maciej

binutils-2.20.51-20100925-mips-gas-ld-tmp.patch
Index: binutils-2.20.51/gas/config/tc-mips.c
===================================================================
--- binutils-2.20.51.orig/gas/config/tc-mips.c
+++ binutils-2.20.51/gas/config/tc-mips.c
@@ -7346,9 +7346,19 @@ macro (struct mips_cl_insn *ip)
 
     case M_LD_OB:
       s = HAVE_64BIT_GPRS ? "ld" : "lw";
+      if (!HAVE_64BIT_GPRS && treg == breg && breg != ZERO)
+	{
+	  /* First load would overwrite the base register, use a
+	     temporary register.  */
+	  used_at = 1;
+	  macro_build (NULL, ADDRESS_ADD_INSN, "d,v,t", AT, breg, ZERO);
+	  breg = AT;
+	}
       goto sd_ob;
+
     case M_SD_OB:
       s = HAVE_64BIT_GPRS ? "sd" : "sw";
+
     sd_ob:
       macro_build (&offset_expr, s, "t,o(b)", treg, BFD_RELOC_LO16, breg);
       if (!HAVE_64BIT_GPRS)

Re: [PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register

[Richard Sandiford]

"Maciej W. Rozycki" <macro@linux-mips.org> writes:
>  Here's another one:
>
> $ cat ld-base.s
> 	ld	$4, ($4)
> $ mips-linux-as -o ld-base.o ld-base.s
> $ mips-linux-objdump -d ld-base.o
>
> ld-base.o:     file format elf32-tradbigmips
>
>
> Disassembly of section .text:
>
> 00000000 <.text>:
>    0:	8c840000 	lw	a0,0(a0)
>    4:	00000000 	nop
>    8:	8c850004 	lw	a1,4(a0)
>
> Ouch!
>
>  Fixed thus and regression tested for mips-linux, mips64-linux, 
> mipstx39-elf, mipsisa64-elf and mips-ecoff targets and their little-endian 
> counterparts.  The new code produced looks like this:
>
> $ mips-linux-objdump -d ld-base.o
>
> ld-base.o:     file format elf32-tradbigmips
>
>
> Disassembly of section .text:
>
> 00000000 <.text>:
>    0:	00800821 	move	at,a0
>    4:	8c240000 	lw	a0,0(at)
>    8:	8c250004 	lw	a1,4(at)

Why are you doing it this way, rather than reversing the loads?
Seems a shame to use $at when we don't need to.

Richard

Re: [PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register

[Maciej W. Rozycki]

On Mon, 18 Oct 2010, Richard Sandiford wrote:

> > Disassembly of section .text:
> >
> > 00000000 <.text>:
> >    0:	00800821 	move	at,a0
> >    4:	8c240000 	lw	a0,0(at)
> >    8:	8c250004 	lw	a1,4(at)
> 
> Why are you doing it this way, rather than reversing the loads?

 Because I've got a hole in my imagination here? :/

> Seems a shame to use $at when we don't need to.

 Indeed, although we're no better elsewhere, consider e.g.:

$ cat ld-base-1.s
	.comm	foo, 1024
	ld	$4, foo
$ as -32 -o ld-base-1.o ld-base-1.s
$ objdump -dr ld-base-1.o

ld-base-1.o:     file format elf32-tradbigmips


Disassembly of section .text:

00000000 <.text>:
   0:	3c010000	lui	at,0x0
			0: R_MIPS_HI16	foo
   4:	8c240000	lw	a0,0(at)
			4: R_MIPS_LO16	foo
   8:	8c250004	lw	a1,4(at)
			8: R_MIPS_LO16	foo
   c:	00000000	nop

where $a1 could be used in place of $at, couldn't it?

 I'll cook up something, though I only have worms to offer it would seem.

  Maciej

[PATCH 1/5] MIPS/GAS: Fix o32 LD to the base register

[Maciej W. Rozycki]

On Mon, 18 Oct 2010, Maciej W. Rozycki wrote:

> > > Disassembly of section .text:
> > >
> > > 00000000 <.text>:
> > >    0:	00800821 	move	at,a0
> > >    4:	8c240000 	lw	a0,0(at)
> > >    8:	8c250004 	lw	a1,4(at)
> > 
> > Why are you doing it this way, rather than reversing the loads?
> 
>  Because I've got a hole in my imagination here? :/
> 
> > Seems a shame to use $at when we don't need to.
> 
>  Indeed, although we're no better elsewhere, consider e.g.:
> 
> $ cat ld-base-1.s
> 	.comm	foo, 1024
> 	ld	$4, foo
> $ as -32 -o ld-base-1.o ld-base-1.s
> $ objdump -dr ld-base-1.o
> 
> ld-base-1.o:     file format elf32-tradbigmips
> 
> 
> Disassembly of section .text:
> 
> 00000000 <.text>:
>    0:	3c010000	lui	at,0x0
> 			0: R_MIPS_HI16	foo
>    4:	8c240000	lw	a0,0(at)
> 			4: R_MIPS_LO16	foo
>    8:	8c250004	lw	a1,4(at)
> 			8: R_MIPS_LO16	foo
>    c:	00000000	nop
> 
> where $a1 could be used in place of $at, couldn't it?

 So this has evolved into a mini-batch now too.  I have verified changes 
in this series with mips-linux, mips64-linux, mipstx39-elf, mipsisa64-elf 
and mips-ecoff targets and their little-endian counterparts (including 
binutils and LD tests too :) ).

 These patches apply on top of the NewABI reloc LD/SD fix where 
applicable.

2010-10-31  Maciej W. Rozycki  <macro@linux-mips.org>

	gas/
	* config/tc-mips.c (macro)[M_LD_OB]: If the first load of the
	pair would overwrite the base register, then swap the accesses.

 OK?

binutils-2.20.51-20100925-mips-gas-ld-o-base.patch
Index: binutils-2.20.51/gas/config/tc-mips.c
===================================================================
--- binutils-2.20.51.orig/gas/config/tc-mips.c
+++ binutils-2.20.51/gas/config/tc-mips.c
@@ -7346,17 +7346,29 @@ macro (struct mips_cl_insn *ip)
 
     case M_LD_OB:
       s = HAVE_64BIT_GPRS ? "ld" : "lw";
+      off = 1;
+      /* If the first load would overwrite the base register,
+         then swap the accesses.  */
+      if (!HAVE_64BIT_GPRS && treg == breg && breg != ZERO)
+	{
+	  offset_expr.X_add_number += 4;
+	  treg += 1;
+	  off = -1;
+	}
       goto sd_ob;
+
     case M_SD_OB:
       s = HAVE_64BIT_GPRS ? "sd" : "sw";
+      off = 1;
+
     sd_ob:
       macro_build (&offset_expr, s, "t,o(b)", treg,
 		   -1, offset_reloc[0], offset_reloc[1], offset_reloc[2],
 		   breg);
       if (!HAVE_64BIT_GPRS)
 	{
-	  offset_expr.X_add_number += 4;
-	  macro_build (&offset_expr, s, "t,o(b)", treg + 1,
+	  offset_expr.X_add_number += 4 * off;
+	  macro_build (&offset_expr, s, "t,o(b)", treg + off,
 		       -1, offset_reloc[0], offset_reloc[1], offset_reloc[2],
 		       breg);
 	}

Re: [PATCH 1/5] MIPS/GAS: Fix o32 LD to the base register

[Richard Sandiford]

"Maciej W. Rozycki" <macro@linux-mips.org> writes:
> binutils-2.20.51-20100925-mips-gas-ld-o-base.patch
> Index: binutils-2.20.51/gas/config/tc-mips.c
> ===================================================================
> --- binutils-2.20.51.orig/gas/config/tc-mips.c
> +++ binutils-2.20.51/gas/config/tc-mips.c
> @@ -7346,17 +7346,29 @@ macro (struct mips_cl_insn *ip)
>  
>      case M_LD_OB:
>        s = HAVE_64BIT_GPRS ? "ld" : "lw";
> +      off = 1;
> +      /* If the first load would overwrite the base register,
> +         then swap the accesses.  */
> +      if (!HAVE_64BIT_GPRS && treg == breg && breg != ZERO)
> +	{
> +	  offset_expr.X_add_number += 4;
> +	  treg += 1;
> +	  off = -1;
> +	}
>        goto sd_ob;
> +
>      case M_SD_OB:
>        s = HAVE_64BIT_GPRS ? "sd" : "sw";
> +      off = 1;
> +

Add a fallthrough comment if you're adding whitespace.  Patches 1-3
are otherwise OK.

I'm not convinced 4 is worthwhile given that these macros are really in
maintenance mode.

I still don't like the way that you're matching ECOFF and ELF offsets
in a single test, allowing two load offsets when (for each format
individually) only one offset is actually acceptable.  But patch 5
is OK nonetheless.

Richard

Re: [PATCH 1/5] MIPS/GAS: Fix o32 LD to the base register

[Richard Sandiford]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> I still don't like the way that you're matching ECOFF and ELF offsets
> in a single test, allowing two load offsets when (for each format
> individually) only one offset is actually acceptable.  But patch 5
> is OK nonetheless.

That is, the relevant parts of 5 are OK nonetheless (i.e. the
parts that cover patch 1).  And I suppose that won't include any
of the ELF/ECOFF thing anyway.

Richard