From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22403 invoked by alias); 4 Jul 2011 22:58:16 -0000 Received: (qmail 22393 invoked by uid 22791); 4 Jul 2011 22:58:15 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD 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; Mon, 04 Jul 2011 22:58:00 +0000 Received: (qmail 28158 invoked from network); 4 Jul 2011 22:57:59 -0000 Received: from unknown (HELO tp.orcam.me.uk) (macro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 4 Jul 2011 22:57:59 -0000 Date: Tue, 05 Jul 2011 00:28:00 -0000 From: "Maciej W. Rozycki" To: Richard Sandiford cc: binutils@sourceware.org Subject: Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code In-Reply-To: <87ei254wmb.fsf@firetop.home> Message-ID: References: <87boxg5nyo.fsf@firetop.home> <87boxdcf2b.fsf@firetop.home> <87ei254wmb.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/msg00047.txt.bz2 On Mon, 4 Jul 2011, Richard Sandiford wrote: > > BTW, I've been wondering about this construct: > > > > return mask & ~0; > > > > you've introduced in a couple of places. > > Oops. Fixed below. Hmm, makes sense indeed, thanks. Except that it didn't pop up in testing which is worrisome and means that all this branch swapping stuff is not really terribly well covered. I think we should cook up something more systematic than the couple of scattered cases we already have. > > Also why "can not" instead of "cannot"? My understanding of the > > subtleties of English is this essentially means we "are allowed not to > > swap" rather than we "are not allowed to swap". The usual disclaimer > > applies of course. ;) > > Those comments were already there. I just moved them around. Fair enough. I got a notion of being otherwise, but now that I have double-checked you are right. Sorry about the confusion. In that case though I'm tempted to adjust the comments I'll be tweaking anyway with the microMIPS changes, if you don't mind that is. > > Finally I think the change below is needed as well. We shouldn't be > > relying on the meaning of FP_D for MIPS16 code as opcode flags are assumed > > to have a different meaning in the MIPS16 table. Granted, the location of > > the FP_D bit hasn't been assigned to anything (yet), so it's guaranteed to > > be zero and the check is harmless, > > Yes. I checked that before writing it this way. It's guaranteed to be > harmless anyway, because "until" MIPS16 gets hardware FP support, we'd > never have any set bits. Actually, I was more worried about this bit being reused for something else as a distinct MIPS16_* flag and then hitting this condition inadvertently. There is a place in append_insn() using the FPR mask unconditionally even in the MIPS16 mode (quite rightfully IMO). Granted, it's only used for the calculation of .fmask, that is less of importance now that we have DWARF-2, but still. How about making a suitable comment in include/opcode/mips.h, preferably following all the other MIPS16_* flags, that the bit position used for the FP_D flag is already reserved for MIPS16 code too? There's a terse description already there covering the shared flags. I think a change like below should be enough. I think INSN_ISA3 should be moved to the beginning and made clearly distinct. It took me a while to notice it's there. I have decided these changes are too trivial to be kept separate, so I've made them both at once. > I structured the code to be consistent with the GPR case. If you feel > strongly about it though, feel free to move the conditions inside the > existing "!mips_opts.mips16" check. I just feel no confidence that the notion of such an assumption will stand for the next half a dozen years or so. The chance for the MIPS16 ASE being extended any further is probably slim, but the lifespan of binutils is long enough to be careful about any assumptions and about long-term maintenance of any piece of code. We seem to keep discovering obscure oddities all the time despite quite a strict maintenance regime. As long as include/opcode/mips.h is updated accordingly I don't insist on my previous change though. Maciej 2011-07-04 Maciej W. Rozycki include/opcode/ * mips.h: Document the use of FP_D in MIPS16 mode. Adjust the order of flags documented. Index: include/opcode/mips.h =================================================================== RCS file: /cvs/src/src/include/opcode/mips.h,v retrieving revision 1.73 diff -u -p -r1.73 mips.h --- include/opcode/mips.h 28 Feb 2011 16:06:51 -0000 1.73 +++ include/opcode/mips.h 4 Jul 2011 22:57:01 -0000 @@ -1132,6 +1132,9 @@ extern int bfd_mips_num_opcodes; /* The following flags have the same value for the mips16 opcode table: + + INSN_ISA3 + INSN_UNCOND_BRANCH_DELAY INSN_COND_BRANCH_DELAY INSN_COND_BRANCH_LIKELY (never used) @@ -1140,7 +1143,7 @@ extern int bfd_mips_num_opcodes; INSN_WRITE_HI INSN_WRITE_LO INSN_TRAP - INSN_ISA3 + FP_D (never used) */ extern const struct mips_opcode mips16_opcodes[];