From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22465 invoked by alias); 29 Jul 2011 09:26:10 -0000 Received: (qmail 22453 invoked by uid 22791); 29 Jul 2011 09:26:07 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_BZ,TW_XD,TW_XF X-Spam-Check-By: sourceware.org Received: from mail-fx0-f41.google.com (HELO mail-fx0-f41.google.com) (209.85.161.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 29 Jul 2011 09:25:51 +0000 Received: by fxg9 with SMTP id 9so2612075fxg.0 for ; Fri, 29 Jul 2011 02:25:49 -0700 (PDT) Received: by 10.223.68.22 with SMTP id t22mr1468069fai.145.1311931549700; Fri, 29 Jul 2011 02:25:49 -0700 (PDT) Received: from richards-thinkpad (gbibp9ph1--blueice3n2.emea.ibm.com [195.212.29.84]) by mx.google.com with ESMTPS id h17sm268263fai.26.2011.07.29.02.25.46 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 29 Jul 2011 02:25:47 -0700 (PDT) From: Richard Sandiford To: "Maciej W. Rozycki" Mail-Followup-To: "Maciej W. Rozycki" ,binutils@sourceware.org, Chao-ying Fu , Rich Fuhler , David Lau , Kevin Mills , Ilie Garbacea , Catherine Moore , Nathan Sidwell , Joseph Myers , rdsandiford@googlemail.com Cc: binutils@sourceware.org, Chao-ying Fu , Rich Fuhler , David Lau , Kevin Mills , Ilie Garbacea , Catherine Moore , Nathan Sidwell , Joseph Myers Subject: Re: [PATCH] MIPS: microMIPS ASE support References: <87y6fa9u3t.fsf@firetop.home> <876302kqvu.fsf@firetop.home> <8739pb1qs5.fsf@firetop.home> <87r5abs7ak.fsf@firetop.home> <87ipqrwyf4.fsf@firetop.home> Date: Fri, 29 Jul 2011 11:30:00 -0000 In-Reply-To: (Maciej W. Rozycki's message of "Fri, 29 Jul 2011 00:52:03 +0100 (BST)") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2011-07/txt/msg00275.txt.bz2 "Maciej W. Rozycki" writes: > Here's what I have come up with as a result of merging your changes into > my code base. There are plenty of small changes, so I have decided not to > put too much effort into straightening them up (or not) lest you are > unhappy with the outcome anyway. Instead, I'm giving you (and the others) > an opportunity to review my code in its current shape. Thanks for doing it this way. > On Tue, 26 Jul 2011, Maciej W. Rozycki wrote: >> There's a whole lot of important linker relaxation fixes that I reckon >> were not included in the original series plus several bug fixes. > > I'll work on these fixes now -- there are quite a few and at least one > still requires some work not to be considered a dirty hack -- and will be > back with you shortly. Thanks. This is what I asked for in the first place: that anything we hadn't caught should be dealt with as follow-ups, rather than folded into the huge initial commit. (For reference, even the .tar.bz2 was over 280k.) I still don't understand why you chose to ignore that. The whole point of approving most of the submission with a few minor changes (back in March) was that it would be quick to make those changes and get the whole thing into CVS. It would then be much easier for me to review any further changes you wanted to make. I also assumed that it would be easier for you to submit them. Anyway, on a more positive note, a lot of the hunks are OK to commit, as follows: > Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c > =================================================================== > --- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2011-07-28 23:18:50.000000000 +0100 > +++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2011-07-28 23:30:26.000000000 +0100 > @@ -483,7 +483,7 @@ static int mips_32bitmode = 0; > (strncmp (TARGET_CPU, "mips16", sizeof ("mips16") - 1) == 0 \ > || strncmp (TARGET_CANONICAL, "mips-lsi-elf", sizeof ("mips-lsi-elf") - 1) == 0) > > -/* Return true if the given CPU supports microMIPS. */ > +/* Return true if the given CPU supports the microMIPS ASE. */ > #define CPU_HAS_MICROMIPS(cpu) 0 > > /* True if CPU has a dror instruction. */ OK > @@ -2141,7 +2141,7 @@ reg_lookup (char **s, unsigned int types > As a special exception if one of s0-s7 registers is specified as > the range's lower delimiter and s8 (fp) is its upper one, then no > registers whose numbers place them between s7 and s8 (i.e. $24-$29) > - are selected; they have to be named separately if needed. */ > + are selected; they have to be listed separately if needed. */ > > static int > reglist_lookup (char **s, unsigned int types, unsigned int *reglistp) OK > @@ -2893,18 +2888,21 @@ relax_end (void) > mips_relax.sequence = 0; > } > > -/* Return the mask of core registers that instruction IP may > - read or write. */ > +/* Return the mask of core registers that IP reads or writes. */ > > static unsigned int > gpr_mod_mask (const struct mips_cl_insn *ip) > { > - unsigned long pinfo, pinfo2; > + unsigned long pinfo2; > unsigned int mask; > > mask = 0; > - pinfo = ip->insn_mo->pinfo; > pinfo2 = ip->insn_mo->pinfo2; > + if (!mips_opts.mips16) > + { > + if (pinfo2 & INSN2_MOD_SP) > + mask |= 1 << SP; > + } > if (mips_opts.micromips) > { > if (pinfo2 & INSN2_MOD_GPR_MB) and: > @@ -2934,8 +2932,6 @@ gpr_mod_mask (const struct mips_cl_insn > mask |= 1 << EXTRACT_OPERAND (1, MP, *ip); > if (pinfo2 & INSN2_MOD_GPR_MQ) > mask |= 1 << micromips_to_32_reg_q_map[EXTRACT_OPERAND (1, MQ, *ip)]; > - if (pinfo2 & INSN2_MOD_SP) > - mask |= 1 << SP; > } > return mask; > } *sigh* Have I introduced another write to an unused variable? :-( I really should use a more modern compiler. The removal of pinfo is OK, thanks. The rest of it doesn't look like an improvement. > @@ -2969,27 +2965,20 @@ gpr_read_mask (const struct mips_cl_insn > if (pinfo & MIPS16_INSN_READ_GPR_X) > mask |= 1 << MIPS16_EXTRACT_OPERAND (REGR32, *ip); > } > - else if (mips_opts.micromips) > - { > - if (pinfo & INSN_READ_GPR_T) > - mask |= 1 << EXTRACT_OPERAND (1, RT, *ip); > - if (pinfo & INSN_READ_GPR_S) > - mask |= 1 << EXTRACT_OPERAND (1, RS, *ip); > - if (pinfo2 & INSN2_READ_GPR_31) > - mask |= 1 << RA; > - if (pinfo2 & INSN2_READ_GP) > - mask |= 1 << GP; > - } > else > { > if (pinfo2 & INSN2_READ_GPR_D) > - mask |= 1 << EXTRACT_OPERAND (0, RD, *ip); > + mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RD, *ip); > if (pinfo & INSN_READ_GPR_T) > - mask |= 1 << EXTRACT_OPERAND (0, RT, *ip); > + mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RT, *ip); > if (pinfo & INSN_READ_GPR_S) > - mask |= 1 << EXTRACT_OPERAND (0, RS, *ip); > + mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RS, *ip); > + if (pinfo2 & INSN2_READ_GP) > + mask |= 1 << GP; > + if (pinfo2 & INSN2_READ_GPR_31) > + mask |= 1 << RA; > if (pinfo2 & INSN2_READ_GPR_Z) > - mask |= 1 << EXTRACT_OPERAND (0, RZ, *ip); > + mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RZ, *ip); > } > /* Don't include register 0. */ > return mask & ~1; OK > @@ -3023,23 +3012,14 @@ gpr_write_mask (const struct mips_cl_ins > if (pinfo & MIPS16_INSN_WRITE_GPR_Y) > mask |= 1 << MIPS16OP_EXTRACT_REG32R (ip->insn_opcode); > } > - else if (mips_opts.micromips) > - { > - if (pinfo & INSN_WRITE_GPR_D) > - mask |= 1 << EXTRACT_OPERAND (1, RD, *ip); > - if (pinfo & INSN_WRITE_GPR_T) > - mask |= 1 << EXTRACT_OPERAND (1, RT, *ip); > - if (pinfo2 & INSN2_WRITE_GPR_S) > - mask |= 1 << EXTRACT_OPERAND (1, RS, *ip); > - if (pinfo & INSN_WRITE_GPR_31) > - mask |= 1 << RA; > - } > else > { > if (pinfo & INSN_WRITE_GPR_D) > mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RD, *ip); > if (pinfo & INSN_WRITE_GPR_T) > mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RT, *ip); > + if (pinfo2 & INSN2_WRITE_GPR_S) > + mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, RS, *ip); > if (pinfo & INSN_WRITE_GPR_31) > mask |= 1 << RA; > if (pinfo2 & INSN2_WRITE_GPR_Z) OK > @@ -3060,19 +3040,10 @@ fpr_read_mask (const struct mips_cl_insn > mask = 0; > pinfo = ip->insn_mo->pinfo; > pinfo2 = ip->insn_mo->pinfo2; > - if (mips_opts.micromips) > + if (!mips_opts.mips16) > { > if (pinfo2 & INSN2_READ_FPR_D) > - mask |= 1 << EXTRACT_OPERAND (1, FD, *ip); > - if (pinfo & INSN_READ_FPR_S) > - mask |= 1 << EXTRACT_OPERAND (1, FS, *ip); > - if (pinfo & INSN_READ_FPR_T) > - mask |= 1 << EXTRACT_OPERAND (1, FT, *ip); > - if (pinfo & INSN_READ_FPR_R) > - mask |= 1 << EXTRACT_OPERAND (1, FR, *ip); > - } > - else if (!mips_opts.mips16) > - { > + mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, FD, *ip); > if (pinfo & INSN_READ_FPR_S) > mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, FS, *ip); > if (pinfo & INSN_READ_FPR_T) OK > @@ -3100,16 +3071,7 @@ fpr_write_mask (const struct mips_cl_ins > mask = 0; > pinfo = ip->insn_mo->pinfo; > pinfo2 = ip->insn_mo->pinfo2; > - if (mips_opts.micromips) > - { > - if (pinfo2 & INSN_WRITE_FPR_D) > - mask |= 1 << EXTRACT_OPERAND (1, FD, *ip); > - if (pinfo & INSN_WRITE_FPR_S) > - mask |= 1 << EXTRACT_OPERAND (1, FS, *ip); > - if (pinfo & INSN_WRITE_FPR_T) > - mask |= 1 << EXTRACT_OPERAND (1, FT, *ip); > - } > - else if (!mips_opts.mips16) > + if (!mips_opts.mips16) > { > if (pinfo & INSN_WRITE_FPR_D) > mask |= 1 << EXTRACT_OPERAND (mips_opts.micromips, FD, *ip); OK > @@ -3720,8 +3683,9 @@ can_swap_branch_p (struct mips_cl_insn * > return FALSE; > > /* If the previous instruction is in a variant frag other than this > - branch's one, we cannot do the swap. This does not apply to the > - mips16, which uses variant frags for different purposes. */ > + branch's one, we cannot do the swap. This does not apply to > + MIPS16/microMIPS code, which uses variant frags for different > + purposes. */ > if (!HAVE_CODE_COMPRESSION > && history[0].frag > && history[0].frag->fr_type == rs_machine_dependent) OK > ip->fixp[0] = fix_new_exp (ip->frag, ip->where, > bfd_get_reloc_size (howto), > address_expr, > - howto->pc_relative, final_type[0]); > + howto0 && howto0->pc_relative, > + final_type[0]); The howto0 variable and null check are OK. Please leave the rest as-is. > Index: binutils-fsf-trunk-quilt/bfd/elfxx-mips.c > =================================================================== > --- binutils-fsf-trunk-quilt.orig/bfd/elfxx-mips.c 2011-07-28 23:18:50.000000000 +0100 > +++ binutils-fsf-trunk-quilt/bfd/elfxx-mips.c 2011-07-28 23:30:26.000000000 +0100 > @@ -11910,8 +11910,7 @@ mips_elf_relax_delete_bytes (bfd *abfd, > symtab_hdr = &elf_tdata (abfd)->symtab_hdr; > isym = (Elf_Internal_Sym *) symtab_hdr->contents; > for (isymend = isym + symtab_hdr->sh_info; isym < isymend; isym++) > - if (isym->st_shndx == sec_shndx > - && isym->st_value > addr) > + if (isym->st_shndx == sec_shndx && isym->st_value > addr) > isym->st_value -= count; > > /* Now adjust the global symbols defined in this section. */ OK > @@ -11989,7 +11987,7 @@ static const struct opcode_descriptor b_ > { /* "b", "mD", */ 0xcc00, 0xfc00 }; > > static const struct opcode_descriptor bz_insn_16 = > - { /* "b(eq|ne)z", "md,mE", */ 0x8c00, 0xac00 }; > + { /* "b(eq|ne)z", "md,mE", */ 0x8c00, 0xdc00 }; > > > /* 32-bit and 16-bit branch EQ and NE zero. */ I don't recall discussing this. Is it a separate thing you noticed? OK regardless. > @@ -12241,7 +12239,7 @@ check_br16 (bfd *abfd, bfd_byte *ptr, un > /* If PTR points to a 32-bit branch or jump that doesn't fiddle with REG, > then return TRUE, otherwise FALSE. */ > > -static int > +static bfd_boolean > check_br32 (bfd *abfd, bfd_byte *ptr, unsigned long reg) > { > unsigned long opcode; Likewise. > @@ -12361,6 +12364,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, > else if (!bfd_malloc_and_get_section (abfd, sec, &contents)) > goto error_return; > } > + ptr = contents + irel->r_offset; > > /* Read this BFD's local symbols if we haven't done so already. */ > if (isymbuf == NULL && symtab_hdr->sh_info != 0) OK > @@ -12432,8 +12436,8 @@ _bfd_mips_elf_relax_section (bfd *abfd, > if (irel->r_offset + 4 > sec->size) > continue; > > - opcode = bfd_get_16 (abfd, contents + irel->r_offset ) << 16; > - opcode |= bfd_get_16 (abfd, contents + irel->r_offset + 2); > + opcode = bfd_get_16 (abfd, ptr ) << 16; > + opcode |= bfd_get_16 (abfd, ptr + 2); > > /* This is the pc-relative distance from the instruction the > relocation is applied to, to the symbol referred. */ OK > @@ -12452,6 +12456,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, > out the offset). */ > if (r_type == R_MICROMIPS_HI16 && MATCH (opcode, lui_insn)) > { > + bfd_boolean bzc = FALSE; > unsigned long nextopc; > unsigned long reg; > bfd_vma offset; and: > @@ -12475,19 +12480,19 @@ _bfd_mips_elf_relax_section (bfd *abfd, > && ELF32_R_SYM (irel[2].r_info) == r_symndx) > continue; > > - /* See if the LUI instruction *might* be in a branch delay slot. */ > + /* See if the LUI instruction *might* be in a branch delay slot. > + We check whether what looks like a 16-bit branch or jump is > + actually an immediate argument to a compact branch, and let > + it through if so. */ > if (irel->r_offset >= 2 > - && check_br16_dslot (abfd, contents + irel->r_offset - 2) > 0 > + && check_br16_dslot (abfd, ptr - 2) > && !(irel->r_offset >= 4 > - /* If the instruction is actually a 4-byte branch, > - the value of check_br16_dslot doesn't matter. > - We should use check_br32_dslot to check whether > - the branch has a delay slot. */ > - && check_4byte_branch (internal_relocs, irelend, > - irel->r_offset - 4))) > + && (bzc = check_bzc (abfd, ptr - 4, irel->r_offset - 4, > + internal_relocs, irelend)))) > continue; > if (irel->r_offset >= 4 > - && check_br32_dslot (abfd, contents + irel->r_offset - 4) > 0) > + && !bzc > + && check_br32_dslot (abfd, ptr - 4)) > continue; > > reg = OP32_SREG (opcode); As regards what you were saying about straightening the patch up: I think this is the one bit that ought to be split out separately and treated as a separate patch. The bzc variable and !bzc check look suspiciously redundant. The use of "ptr" is OK now though. > @@ -12502,11 +12507,11 @@ _bfd_mips_elf_relax_section (bfd *abfd, > case 0: > break; > case 2: > - if (check_br16 (abfd, contents + irel->r_offset + 4, reg)) > + if (check_br16 (abfd, ptr + 4, reg)) > break; > continue; > case 4: > - if (check_br32 (abfd, contents + irel->r_offset + 4, reg)) > + if (check_br32 (abfd, ptr + 4, reg)) > break; > continue; > default: OK > @@ -12581,8 +12586,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, > && irel->r_offset + 5 < sec->size > && ((fndopc = find_match (opcode, bz_rs_insns_32)) >= 0 > || (fndopc = find_match (opcode, bz_rt_insns_32)) >= 0) > - && MATCH (bfd_get_16 (abfd, contents + irel->r_offset + 4), > - nop_insn_16)) > + && MATCH (bfd_get_16 (abfd, ptr + 4), nop_insn_16)) > { > unsigned long reg; > OK > @@ -12593,10 +12597,8 @@ _bfd_mips_elf_relax_section (bfd *abfd, > | BZC32_REG_FIELD (reg) > | (opcode & 0xffff)); /* Addend value. */ > > - bfd_put_16 (abfd, (opcode >> 16) & 0xffff, > - contents + irel->r_offset); > - bfd_put_16 (abfd, opcode & 0xffff, > - contents + irel->r_offset + 2); > + bfd_put_16 (abfd, (opcode >> 16) & 0xffff, ptr); > + bfd_put_16 (abfd, opcode & 0xffff, ptr + 2); > > /* Delete the 16-bit delay slot NOP: two bytes from > irel->offset + 4. */ OK > @@ -12617,7 +12619,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, > bfd_put_16 (abfd, > (b_insn_16.match > | (opcode & 0x3ff)), /* Addend value. */ > - contents + irel->r_offset); > + ptr); > > /* Delete 2 bytes from irel->r_offset + 2. */ > delcnt = 2; OK > @@ -12645,7 +12647,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, > (bz_insns_16[fndopc].match > | BZ16_REG_FIELD (reg) > | (opcode & 0x7f)), /* Addend value. */ > - contents + irel->r_offset); > + ptr); > > /* Delete 2 bytes from irel->r_offset + 2. */ > delcnt = 2; OK > @@ -12661,14 +12663,13 @@ _bfd_mips_elf_relax_section (bfd *abfd, > unsigned long n32opc; > bfd_boolean relaxed = FALSE; > > - n32opc = bfd_get_16 (abfd, contents + irel->r_offset + 4) << 16; > - n32opc |= bfd_get_16 (abfd, contents + irel->r_offset + 6); > + n32opc = bfd_get_16 (abfd, ptr + 4) << 16; > + n32opc |= bfd_get_16 (abfd, ptr + 6); > > if (MATCH (n32opc, nop_insn_32)) > { > /* Replace delay slot 32-bit NOP with a 16-bit NOP. */ > - bfd_put_16 (abfd, nop_insn_16.match, > - contents + irel->r_offset + 4); > + bfd_put_16 (abfd, nop_insn_16.match, ptr + 4); > > relaxed = TRUE; > } OK > @@ -12679,7 +12680,7 @@ _bfd_mips_elf_relax_section (bfd *abfd, > (move_insn_16.match > | MOVE16_RD_FIELD (MOVE32_RD (n32opc)) > | MOVE16_RS_FIELD (MOVE32_RS (n32opc))), > - contents + irel->r_offset + 4); > + ptr + 4); > > relaxed = TRUE; > } OK > @@ -12691,9 +12692,9 @@ _bfd_mips_elf_relax_section (bfd *abfd, > /* JAL with 32-bit delay slot that is changed to a JALS > with 16-bit delay slot. */ > bfd_put_16 (abfd, (jal_insn_32_bd16.match >> 16) & 0xffff, > - contents + irel->r_offset); > + ptr); > bfd_put_16 (abfd, jal_insn_32_bd16.match & 0xffff, > - contents + irel->r_offset + 2); > + ptr + 2); > > /* Delete 2 bytes from irel->r_offset + 6. */ > delcnt = 2; OK > Index: binutils-fsf-trunk-quilt/include/opcode/mips.h > =================================================================== > --- binutils-fsf-trunk-quilt.orig/include/opcode/mips.h 2011-07-28 23:29:25.000000000 +0100 > +++ binutils-fsf-trunk-quilt/include/opcode/mips.h 2011-07-28 23:30:26.000000000 +0100 > @@ -1332,13 +1332,10 @@ extern int bfd_mips_num_opcodes; > extern const struct mips_opcode mips16_opcodes[]; > extern const int bfd_mips16_num_opcodes; > > -/* These are the bitmasks and shift counts used for the different > - fields in the instruction formats. Other than MAJOR, no masks are > - provided for the fixed portions of an instruction, since they are > - not needed. */ > +/* These are the bit masks and shift counts used for the different fields > + in the microMIPS instruction formats. No masks are provided for the > + fixed portions of an instruction, since they are not needed. */ > > -#define MICROMIPSOP_MASK_MAJOR 0x3f > -#define MICROMIPSOP_SH_MAJOR 26 > #define MICROMIPSOP_MASK_IMMEDIATE 0xffff > #define MICROMIPSOP_SH_IMMEDIATE 0 > #define MICROMIPSOP_MASK_DELTA 0xffff OK Thanks, Richard