From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12505 invoked by alias); 29 Jul 2011 17:40:31 -0000 Received: (qmail 12492 invoked by uid 22791); 29 Jul 2011 17:40:30 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,TW_BZ,TW_XD,TW_XF X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 29 Jul 2011 17:40:14 +0000 Received: (qmail 19153 invoked from network); 29 Jul 2011 17:40:13 -0000 Received: from unknown (HELO tp.orcam.me.uk) (macro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 Jul 2011 17:40:13 -0000 Date: Fri, 29 Jul 2011 22:52:00 -0000 From: "Maciej W. Rozycki" To: Richard Sandiford 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 In-Reply-To: Message-ID: References: <87y6fa9u3t.fsf@firetop.home> <876302kqvu.fsf@firetop.home> <8739pb1qs5.fsf@firetop.home> <87r5abs7ak.fsf@firetop.home> <87ipqrwyf4.fsf@firetop.home> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) 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/msg00289.txt.bz2 On Fri, 29 Jul 2011, Richard Sandiford 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. Well, I believed this was really what I did -- with all these binutils-ld-lib.diff, binutils-umips-fix.diff, binutils-umips-opcode-trap.diff, binutils-umips-relax16.diff, binutils-umips-fix-reloc.diff patches (and all the outstanding ones that add up to six right now, but will probably become seven or eight) -- but if that wasn't what you intended, then I apologise for misunderstanding. > 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. The problem was I was pulled off to another project as a matter of emergency -- apart from the update to take your recent register use tracking changes into account I did last week, I haven't really touched the pieces for a couple of months now. Sorry about that. I'll go on to explain the bits you had anything but "OK" to say about, commit the bits you are happy with and then we can continue with the rest. > > @@ -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. For the sake of consistency I put all the pinfo/2 flag interpretation that does not involve microMIPS-specific structures (i.e. micromips_to_32_reg_*_map) into (!mips_opts.mips16) rather than (mips_opts.micromips) condition blocks under the assumption we don't have to imply their microMIPS-ness here. Obviously this includes the INSN2_MOD_SP flag as it's self-contained in that it determines the register accessed itself, though it probably has little chance to be useful for non-microMIPS code even with possible future extensions to the base architecture. Therefore I won't insist on this change even though it's consistent with the usage of INSN2_READ_GP and INSN2_READ_GPR_31 flags in gpr_read_mask(). > > 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. There's no need to reiterate checking for non-BFD_RELOC_UNUSED that's been already done earlier on. I'll split it off for further consideration. > > @@ -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. Well, there's nothing to discuss here -- it's a plain bug, the old mask is wrong and there was no need to handle it separately -- in fact the opposite was the case as it makes no sense to check in known-buggy code. I'll split it off and commit separately for clarity. > > @@ -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. I'll split it off too. > > @@ -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. It's a small optimisation -- if we've established for sure the instruction before is a compact branch, then there's no need to go through the hoops and check if it's an ordinary branch too -- which it will never be. I'll split this change off of course. Thanks for the quick review. Maciej