From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 779 invoked by alias); 5 Jun 2010 09:17:14 -0000 Received: (qmail 771 invoked by uid 22791); 5 Jun 2010 09:17:13 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM X-Spam-Check-By: sourceware.org Received: from mail-wy0-f169.google.com (HELO mail-wy0-f169.google.com) (74.125.82.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 05 Jun 2010 09:17:07 +0000 Received: by wyb42 with SMTP id 42so525198wyb.0 for ; Sat, 05 Jun 2010 02:17:04 -0700 (PDT) Received: by 10.227.69.142 with SMTP id z14mr11692841wbi.195.1275729424244; Sat, 05 Jun 2010 02:17:04 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id z33sm17685266wbd.7.2010.06.05.02.17.02 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sat, 05 Jun 2010 02:17:02 -0700 (PDT) From: Richard Sandiford To: "Maciej W. Rozycki" Mail-Followup-To: "Maciej W. Rozycki" ,binutils@sourceware.org, Chao-ying Fu , Catherine Moore , "Joseph S. Myers" , Daniel Jacobowitz , rdsandiford@googlemail.com Cc: binutils@sourceware.org, Chao-ying Fu , Catherine Moore , "Joseph S. Myers" , Daniel Jacobowitz Subject: Re: [PATCH] MIPS: microMIPS and MCU ASE instruction set support References: <87y6fa9u3t.fsf@firetop.home> Date: Sat, 05 Jun 2010 09:17:00 -0000 In-Reply-To: (Maciej W. Rozycki's message of "Tue, 1 Jun 2010 15:19:24 +0100 (BST)") Message-ID: <87fx11bzzm.fsf@firetop.home> 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: 2010-06/txt/msg00063.txt.bz2 "Maciej W. Rozycki" writes: > I'll look into whatever's left yet. Here's the current version for a > reference, in case you or anyone else wanted to comment on what I have > modified so far (no obligation, of course!). OK, finally got chance to have a proper read of the changes from the previous patch, and they look good, thanks. I'm afraid most of this is at the level of very minor niggly stuff: +/* Whether code compression (either of the MIPS16 or the microMIPS ASEs has s/ASEs has/ASEs) has/ + if (mips_opts.micromips) + return is_micromips_16bit_p (insn->insn_mo) + ? 2 : (is_micromips_32bit_p (insn->insn_mo) ? 4 : 6); This formatting isn't quite right because of the "emacs brackets" rule. Should be: return (is_micromips_16bit_p (insn->insn_mo) ? 2 : is_micromips_32bit_p (insn->insn_mo) ? 4 : 6); or: return (is_micromips_16bit_p (insn->insn_mo) ? 2 : is_micromips_32bit_p (insn->insn_mo) ? 4 : 6); But I'd be happy with an if-return-if-return-return chain too. See below about these insns though... -+ if (!mips_opts.mips16 && !mips_opts.micromips) ++ if (! HAVE_CODE_COMPRESSION) The GCC decision a few years back was that no space should be added after a unary operator (as with pointer deference, etc). Not important enough to do a sed on the whole source base, but we might as well avoid changes that go the other way (from no space to space) in GAS. +static bfd_boolean +is_size_valid (const struct mips_opcode *mo) +{ + gas_assert (mips_opts.micromips); + + if ((micromips_16 || micromips_32) && mo->pinfo == INSN_MACRO) + return FALSE; + if (micromips_16 && ! is_micromips_16bit_p (mo)) + return FALSE; + if (micromips_32 && ! is_micromips_32bit_p (mo)) + return FALSE; + + return TRUE; +} Hmm, seeing this highlighted more makes me wonder whether micromips_16 and micromips_32 shouldn't be combined into a single variable that represents "the size the user set", with 0 meaning "none". As it stands, we'll have checks like: micromips_16 || micromips_32 || micromips_48 when any future 48-bit support is added. Having separate variables also gives the impression that arbitrary combinations are possible. Also, how about replacing is_micromips_XXbit_p with a function that just returns the size of a micromips insn? We generally deal with byte rather than bit lengths, so both this new function and the combined "the size the user set" variable should probably both be byte values. Seems the code would be a fair bit clearer with those changes, but maybe I'm wrong... Maybe the mo->match assertions in is_micromips_XXbit_p are better done in validate_micromips_insn -- especially if that makes the changes above easier -- but I don't mind either way. char mips_nop_opcode (void) { if (seg_info (now_seg)->tc_segment_info_data.micromips) return NOP_OPCODE_MICROMIPS; return seg_info (now_seg)->tc_segment_info_data.mips16 ? NOP_OPCODE_MIPS16 : NOP_OPCODE_MIPS; } Same "emacs brackets" thing here. Seems odd to treat microMIPS and MIPS16 differently like this, so I think if-return-if-return-return makes more sense. The new mips_handle_align looks very good, thanks. I'm afraid it's another silly nitpick, but usual style is not to have the brackets in "*(p++)". Richard