* [PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register @ 2010-10-17 16:22 Maciej W. Rozycki 2010-10-18 7:48 ` Richard Sandiford 0 siblings, 1 reply; 6+ messages in thread From: Maciej W. Rozycki @ 2010-10-17 16:22 UTC (permalink / raw) To: Richard Sandiford; +Cc: binutils 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register 2010-10-17 16:22 [PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register Maciej W. Rozycki @ 2010-10-18 7:48 ` Richard Sandiford 2010-10-18 17:05 ` Maciej W. Rozycki 0 siblings, 1 reply; 6+ messages in thread From: Richard Sandiford @ 2010-10-18 7:48 UTC (permalink / raw) To: Maciej W. Rozycki; +Cc: binutils "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register 2010-10-18 7:48 ` Richard Sandiford @ 2010-10-18 17:05 ` Maciej W. Rozycki 2010-10-31 22:46 ` [PATCH 1/5] " Maciej W. Rozycki 0 siblings, 1 reply; 6+ messages in thread From: Maciej W. Rozycki @ 2010-10-18 17:05 UTC (permalink / raw) To: Richard Sandiford; +Cc: binutils 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/5] MIPS/GAS: Fix o32 LD to the base register 2010-10-18 17:05 ` Maciej W. Rozycki @ 2010-10-31 22:46 ` Maciej W. Rozycki 2010-11-01 21:28 ` Richard Sandiford 0 siblings, 1 reply; 6+ messages in thread From: Maciej W. Rozycki @ 2010-10-31 22:46 UTC (permalink / raw) To: Richard Sandiford; +Cc: binutils 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); } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] MIPS/GAS: Fix o32 LD to the base register 2010-10-31 22:46 ` [PATCH 1/5] " Maciej W. Rozycki @ 2010-11-01 21:28 ` Richard Sandiford 2010-11-01 21:34 ` Richard Sandiford 0 siblings, 1 reply; 6+ messages in thread From: Richard Sandiford @ 2010-11-01 21:28 UTC (permalink / raw) To: Maciej W. Rozycki; +Cc: binutils "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] MIPS/GAS: Fix o32 LD to the base register 2010-11-01 21:28 ` Richard Sandiford @ 2010-11-01 21:34 ` Richard Sandiford 0 siblings, 0 replies; 6+ messages in thread From: Richard Sandiford @ 2010-11-01 21:34 UTC (permalink / raw) To: Maciej W. Rozycki; +Cc: binutils 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-11-01 21:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-10-17 16:22 [PATCH 1/2] MIPS/GAS: Fix o32 LD to the base register Maciej W. Rozycki 2010-10-18 7:48 ` Richard Sandiford 2010-10-18 17:05 ` Maciej W. Rozycki 2010-10-31 22:46 ` [PATCH 1/5] " Maciej W. Rozycki 2010-11-01 21:28 ` Richard Sandiford 2010-11-01 21:34 ` Richard Sandiford
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).