From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20912 invoked by alias); 14 Mar 2013 20:55:11 -0000 Received: (qmail 20904 invoked by uid 22791); 14 Mar 2013 20:55:10 -0000 X-SWARE-Spam-Status: No, hits=-3.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_QE X-Spam-Check-By: sourceware.org Received: from mail-wi0-f173.google.com (HELO mail-wi0-f173.google.com) (209.85.212.173) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 14 Mar 2013 20:55:04 +0000 Received: by mail-wi0-f173.google.com with SMTP id hq4so4212860wib.0 for ; Thu, 14 Mar 2013 13:55:02 -0700 (PDT) X-Received: by 10.194.88.138 with SMTP id bg10mr7040604wjb.13.1363294502487; Thu, 14 Mar 2013 13:55:02 -0700 (PDT) Received: from localhost ([2.26.173.20]) by mx.google.com with ESMTPS id n2sm11265440wiy.6.2013.03.14.13.55.00 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 14 Mar 2013 13:55:01 -0700 (PDT) From: Richard Sandiford To: "Moore\, Catherine" Mail-Followup-To: "Moore\, Catherine" ,"gcc-patches\@gcc.gnu.org" , "Rozycki\, Maciej" , rdsandiford@googlemail.com Cc: "gcc-patches\@gcc.gnu.org" , "Rozycki\, Maciej" Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support References: <87y5mfjm4c.fsf@talisman.home> <871ubukhs7.fsf@talisman.default> <87d2vdtv3x.fsf@talisman.default> Date: Thu, 14 Mar 2013 20:55:00 -0000 In-Reply-To: (Catherine Moore's message of "Wed, 13 Mar 2013 20:30:13 +0000") Message-ID: <871ubhem70.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2013-03/txt/msg00526.txt.bz2 "Moore, Catherine" writes: >> -----Original Message----- >> From: Richard Sandiford [mailto:rdsandiford@googlemail.com] >> Sent: Tuesday, March 05, 2013 4:06 PM >> To: Moore, Catherine >> Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej >> Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support: >> >> We have a few internal-only undocumented constraints that aren't used >> much, so we should be able to move them to the "Y" space instead. The >> patch below does this for "T" and "U". Then we could use "U" for new, >> longer constraints. >> >> >> U >> >> where is: >> >> s for signed >> u for unsigned >> d for decremented unsigned (-1 ... N) >> i for incremented unsigned (1 ... N) >> >> where is: >> >> b for "byte" (*1) >> h for "halfwords" (*2) >> w for "words" (*4) >> d for "doublewords" (*8) -- useful for 64-bit MIPS16 but probably not >> needed for 32-bit microMIPS >> >> and where is the number of bits. and could be >> replaced with an ad-hoc two-letter combination for special cases. >> E.g. "Uas9" ("add stack") for ADDISUP. >> >> Just a suggestion though. I'm not saying these names are totally intuitive or >> anything, but they should at least be better than arbitrary letters. >> >> Also, could be two digits if necessary, or we could just use hex digits. > > I extended this proposal a bit by: > 1. Adding a e for encoded. The constraint will start with Ue, > when the operand is an encoded value. > 2. I decided to use two digits for . > 3. The ad-hoc combination is used for anything else. First of all, thanks for coming up with a counter-suggestion. I'm hopeless at naming things, so I was hoping there would be at least some pushback. "e" for "encoded" sounds good. I'm less keen on the mixture of single- and double-digit widths though (single digit for some "Ue"s, double digits for other "U"s.) I think we should either: (a) use the same number of digits for all "U" constraints. That leaves one character for the "Ue" type, which isn't as mnemonic, but is in line with what we do elsewhere. (b) avoid digits in the "Ue" forms and just have an ad-hoc letter combination. Please keep "U" for constants though. The memory constraints should go under "Z" instead (and therefore be only two letters long). The idea is that the first letter of the constraint tells you what type it is. I don't think there's any need for the "Ue" constraints to have predicates of the same name. We can go with longer, mnemonic, names instead. The idea behind suggesting "sb4_operand", etc., was that (a) every character was predictable and (b) I'm not sure the extra verbosity of (say) "signed_byte_4_operand" would help. But "addiur2_operand" would be good. > +(define_constraint "Udb07" > + "@internal > + A decremented unsigned constant of 7 bits." > + (match_operand 0 "Udb07_operand" "")) Very minor nit, but these "" are redundant. > +(define_constraint "Ueim4" > + "@internal > + A microMIPS encoded ADDIUR2 immediate operand." > + (match_operand 0 "Ueim4_operand" "")) Again minor, but the name doesn't really seem to match the description. Is this constraint needed for things other than ADDIUR2? If so, it might be worth giving a second example, otherwise it might be better to make the name a bit less general. Unless this name comes from the manual, of course :-) (The microMIPS link on the MIPS website was still broken last time I checked, but I haven't tried it again in the last couple of weeks.) > +(define_predicate "Umem0_operand" > + (and (match_code "mem") > + (match_test "umips_lwsp_swsp_address_p (XEXP (op, 0), mode)"))) > + > +(define_predicate "Uload_operand" > + (and (match_code "mem") > + (match_test "umips_address_p (XEXP (op, 0), true, mode)"))) > + > +(define_predicate "Ustore_operand" > + (and (match_code "mem") > + (match_test "umips_address_p (XEXP (op, 0), false, mode)"))) With the two-letter Z constraints, these should have descriptive names. > +(define_predicate "Udb07_operand" > + (and (match_code "const_int") > + (match_test "mips_unsigned_immediate_p (INTVAL (op) + 1, 7, 0)"))) Please drop the "U"s in the predicate names. > +(define_attr "compression" "none,all,micromips,mips16" > + (const_string "none")) Thinking about it a bit more, it would probably be better to leave the "all" and "mips16" values out until we need them. > +(define_attr "enabled" "no,yes" > + (if_then_else (ior (eq_attr "compression" "all") > + (eq_attr "compression" "none") > + (and (eq_attr "compression" "micromips") > + (match_test "TARGET_MICROMIPS"))) > + (const_string "yes") > + (const_string "no"))) FWIW (probably not much after the above), it's also possible to write: (eq_attr "compression" "all,none") > @@ -5237,6 +5271,12 @@ > return "\t%0,%1,%2"; > } > [(set_attr "type" "shift") > + (set_attr_alternative "compression" > + [(if_then_else (ior (match_test "GET_CODE (SET_SRC (PATTERN (insn))) == ASHIFT") > + (match_test "GET_CODE (SET_SRC (PATTERN (insn))) == LSHIFTRT")) > + (const_string "micromips") > + (const_string "none")) > + (const_string "none")]) > (set_attr "mode" "")]) This would probably be better as: (define_code_attr shift_compression [(ashift "micromips") (lshiftrt "micromips") (ashiftrt "none")]) ... (set_attr "compression" "micromips,") which makes the distinction at compile time. The mips.md changes look good otherwise though. One idea (just as a nice-to-have) is that we could add an option that makes the asm output routines add ".16" to mnemonics that we think are 16 bits long, as a bit of extra sanity checking. That would be a separate patch though and is definitely not a requirement. > +/* Return true if X is a legitimate address that conforms to the requirements > + for any of the short microMIPS LOAD or STORE instructions except LWSP > + or SWSP. */ > + > +bool > +umips_address_p (rtx x, bool load, enum machine_mode mode) > +{ > + > + struct mips_address_info addr; > + bool ok = mips_classify_address (&addr, x, mode, false) > + && addr.type == ADDRESS_REG > + && M16_REG_P (REGNO (addr.reg)) > + && CONST_INT_P (addr.offset); > + > + if (!ok) > + return false; > + > + if (mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2) > + || mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 1)) > + return true; > + > + if (load && IN_RANGE (INTVAL (addr.offset), -1, 14)) > + return true; > + > + if (!load && IN_RANGE (INTVAL (addr.offset), 0, 15)) > + return true; > + > + return false; > + > +} No blank lines after "{" and before "}". More importantly, what cases are these range tests covering? Both binutils and qemu seem to think that LW and SW need offsets that exactly match: mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2) i.e.: bool umips_address_p (rtx x, enum machine_mode mode) { struct mips_address_info addr; if (mips_classify_address (&addr, x, mode, false) && addr.type == ADDRESS_REG && M16_REG_P (REGNO (addr.reg)) && uw4_operand (addr.offset)) return true; } with no load vs. store check. > +/* Return true if X is a legitimate address that conforms to the requirements > + for a microMIPS LWSP or SWSP insn. */ > + > +bool > +umips_lwsp_swsp_address_p (rtx x, enum machine_mode mode) > +{ > + struct mips_address_info addr; > + > + return (mips_classify_address (&addr, x, mode, false) > + && addr.type == ADDRESS_REG > + && REGNO (addr.reg) == STACK_POINTER_REGNUM > + && CONST_INT_P (addr.offset) > + && mips_unsigned_immediate_p (INTVAL (addr.offset), 5, 2)); I'd prefer: return (mips_classify_address (&addr, x, mode, false) && addr.type == ADDRESS_REG && REGNO (addr.reg) == STACK_POINTER_REGNUM && uw5_operand (addr.offset)); > @@ -8010,9 +8075,17 @@ mips_print_operand_punctuation (FILE *file, int ch > > case '!': > /* When final_sequence is 0, the delay slot will be a nop. We can > - a 16-bit delay slot for microMIPS. */ > + use a 16-bit delay slot for microMIPS. */ > if (final_sequence == 0) > putc ('s', file); > + else > + { > + /* Use the compact version for microMIPS if the delay slot is a > + compact microMIPS instruction. */ > + rtx insn = XVECEXP (final_sequence, 0, 1); > + if (get_attr_length (insn) == 2) > + putc ('s', file); > + } I think this is easier as: if (final_sequence == 0 || get_attr_length (XVECEXP (final_sequence, 0, 1)) == 2) putc ('s', file); Richard