From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27949 invoked by alias); 13 Feb 2013 11:43:10 -0000 Received: (qmail 27941 invoked by uid 22791); 13 Feb 2013 11:43:09 -0000 X-SWARE-Spam-Status: No, hits=-2.9 required=5.0 tests=AWL,BAYES_20,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_EQ,TW_MG,TW_SW X-Spam-Check-By: sourceware.org Received: from mail-we0-f182.google.com (HELO mail-we0-f182.google.com) (74.125.82.182) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 13 Feb 2013 11:42:47 +0000 Received: by mail-we0-f182.google.com with SMTP id t57so913607wey.41 for ; Wed, 13 Feb 2013 03:42:45 -0800 (PST) X-Received: by 10.180.100.169 with SMTP id ez9mr9483386wib.3.1360755765042; Wed, 13 Feb 2013 03:42:45 -0800 (PST) Received: from localhost ([2.26.176.154]) by mx.google.com with ESMTPS id n2sm42029649wiy.6.2013.02.13.03.42.42 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 13 Feb 2013 03:42:43 -0800 (PST) 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> <87622noebh.fsf@talisman.default> Date: Wed, 13 Feb 2013 11:43:00 -0000 In-Reply-To: (Catherine Moore's message of "Tue, 12 Feb 2013 19:02:04 +0000") Message-ID: <87ip5wv3rj.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (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-02/txt/msg00586.txt.bz2 "Moore, Catherine" writes: >> > Index: config/mips/mips.c >> > >> ========================================================== >> ========= >> > --- config/mips/mips.c (revision 195351) >> > +++ config/mips/mips.c (working copy) >> > @@ -77,6 +77,9 @@ along with GCC; see the file COPYING3. If not see >> > preserve the maximum stack alignment. We therefore use a value >> > of 0x7ff0 in this case. >> > >> > + microMIPS LWM and SWM support 12-bit offsets (from -0x800 to 0x7ff), >> > + so we use a maximum of 0x7f0 for TARGET_MICROMIPS. >> > + >> > MIPS16e SAVE and RESTORE instructions can adjust the stack pointer by >> > up to 0x7f8 bytes and can usually save or restore all the registers >> > that we need to save or restore. (Note that we can only use these >> > @@ -87,7 +90,8 @@ along with GCC; see the file COPYING3. If not see >> > to save and restore registers, and to allocate and deallocate the top >> > part of the frame. */ >> > #define MIPS_MAX_FIRST_STACK_STEP >> \ >> > - (!TARGET_MIPS16 ? 0x7ff0 \ >> > + ((!TARGET_MIPS16 && !TARGET_MICROMIPS) ? 0x7ff0 >> \ >> > + : TARGET_MICROMIPS ? 0x7f0 >> \ >> > : GENERATE_MIPS16E_SAVE_RESTORE ? 0x7f8 >> \ >> > : TARGET_64BIT ? 0x100 : 0x400) >> >> I'd prefer: >> >> (TARGET_MICROMIPS ? 0x7f0 \ >> : !TARGET_MIPS16 ? 0x7ff0 \ >> : GENERATE_MIPS16E_SAVE_RESTORE ? 0x7f8 \ >> : TARGET_64BIT ? 0x100 : 0x400) Thanks for doing this, but... >> The comment doesn't really explain the reason for 0x7f0 rather than 0x7f8. >> Is this in anticipation of future n32 and n64 support, where the stack must be >> 16-byte aligned? If so, that's OK, but worth mentioning. ...it looks like you haven't addressed this part. What I mean is: the stack alignment is 8 bytes rather than 16 bytes on 32-bit ABIs, so I would have expected 0x7f8 rather than 0x7f0, as for the MIPS16 case quoted above. I wasn't sure why you went for 0x7f0 instead. I'm not saying it must be 0x7f8, just that the comment ought to give the reason for using 0x7f0 instead. >> > +; For movep >> > +(define_peephole2 >> > + [(set (match_operand:MOVEP1 0 "register_operand" "") >> > + (match_operand:MOVEP1 1 "movep_operand" "")) >> > + (set (match_operand:MOVEP2 2 "register_operand" "") >> > + (match_operand:MOVEP2 3 "movep_operand" ""))] >> > + "TARGET_MICROMIPS >> > + && umips_movep_target_p (operands[0], operands[2])" >> > + [(parallel [(set (match_dup 0) (match_dup 1)) >> > + (set (match_dup 2) (match_dup 3))])] >> > +) >> >> Probably not worth having movep_operand, because all the checking is done >> by umips_movep_target_p. reg_or_0_operand should be OK. > > The source and target register criteria are not identical. I've now > renamed the movep_operand predicate to movep_src_operand. Ah, yeah, sorry. >> > + /* If DECL is "nocompression" set the "nomips16" and >> > + "nomicromips" attributes. */ >> > + if (nocompression_p & MASK_MIPS16 >> > + && nocompression_p & MASK_MICROMIPS) >> > + { >> > + name = "nomips16"; >> > + *attributes = tree_cons (get_identifier (name), NULL, *attributes); >> > + name = "nomicromips"; >> > + *attributes = tree_cons (get_identifier (name), NULL, *attributes); >> > + } >> >> Hmm, why is this necessary? Anything that cares should be using >> mips_get_compress_off_flags. It looks like you haven't addressed this part. Why do we need to add tree-level "nomips16" and "nomicromips" attributes to a function that has "nocompression"? >> > + && addr.type == ADDRESS_REG >> > + && CONST_INT_P (addr.offset) >> > + && UMIPS_12BIT_OFFSET_P (addr.offset)); >> >> Isn't there a missing INTVAL here? Please check the patch to make sure >> there are no compiler warnings. This too. The point is that addr.offset is an rtx, while UMIPS_12BIT_OFFSET_P takes a HOST_WIDE_INT: #define UMIPS_12BIT_OFFSET_P(OFFSET) (IN_RANGE (OFFSET, -2048, 2047)) So you're comparing an rtx pointer against [-2048, 2047], rather than the integer that the rtx contains. As it stands, I don't see how umips_12bit_offset_address_p would ever return true on normal hosts. That in turn should mean that ZC and ZD never accept anything for microMIPS, so I would have expected this to show up as a reload failure during the microMIPS testsuite run. Please add tests that ZC and ZD work for microMIPS. E.g. the ZC one could be something like: MICROMIPS void foo (int *x) { asm volatile ("insn1\t%0" :: "ZC" (x[0])); asm volatile ("insn2\t%0" :: "ZC" (x[511])); asm volatile ("insn3\t%0" :: "ZC" (x[512])); } /* { dg-final { scan-assembler "\tinsn1\t0\\(" } } */ /* { dg-final { scan-assembler "\tinsn2\t2044\\(" } } */ /* { dg-final { scan-assembler-not "\tinsn3\t2048\\(" } } */ Completely untested :-) Similar idea for ZD. > +@item -minterlink-compressed > +@item -mno-interlink-compressed > +@item -minterlink-uncompressed > +@item -mno-interlink-uncompressed > +@opindex minterlink-compressed > +@opindex mno-interlink-compressed > +@opindex minterlink-compressed > +@opindex mno-interlink-compressed > +Require (do not require) that code using the standard (uncompressed) MIPS ISA > +be link-compatible with MIPS16 and microMIPS code, and vice versa. What I meant in my previous reply was that there should be only one set of options. I.e. everything would use -minterlink-compressed and -mno-interlink-compressed. There would be no -minterlink-uncompressed/ -mno-interlink-uncompressed Sorry for going back on the earlier -mno-jals suggestion here, but I hadn't realised until I saw the previous patch just how confusing it would be to have both. > +@item ZC > +When compiling microMIPS code, this constraing matches a memory operand s/constraing/constraint/ > +@item ZD > +When compiling microMIPS code, this constraint matches an address operand > +that is formed from a base register and a 12-bit offset. These operands > +can be used for microMIPS instructions such as @code{prefetch}. When > +not compiling for microMIPS code, @code{YC} is equivalent to @code{p}. s/YC/ZD/ > Index: config/mips/gnu-user.h > =================================================================== > --- config/mips/gnu-user.h (revision 195900) > +++ config/mips/gnu-user.h (working copy) > @@ -137,3 +137,12 @@ > #define ENDFILE_SPEC \ > GNU_USER_TARGET_MATHFILE_SPEC " " \ > GNU_USER_TARGET_ENDFILE_SPEC > + > +#undef SUBTARGET_OVERRIDE_OPTIONS > +#define SUBTARGET_OVERRIDE_OPTIONS \ > +do { \ > + /* microMIPS PLT entries are non-microMIPS. */ \ > + TARGET_INTERLINK_UNCOMPRESSED = 1; \ > +} while (0) Please leave this out. As discussed, we'll rely on Maciej's linker patch to handle PLTs. > Index: config/mips/micromips.md > =================================================================== > --- config/mips/micromips.md (revision 0) > +++ config/mips/micromips.md (revision 0) > @@ -0,0 +1,108 @@ > +(define_insn "*store_word_multiple" Sorry, forgot to say earlier, but this needs a copyright header. Also: ;; The behaviour of SWM is undefined if placed in a delay slot. (Sorry for the extremely minor nit, but I prefer caps for mnemonics, like in the manual. Please update the others too.) > +(define_insn "*load_word_multiple" > + [(match_parallel 0 "" > + [(set (match_operand:SI 1 "register_operand") > + (match_operand:SI 2 "memory_operand"))])] > + "TARGET_MICROMIPS > + && umips_save_restore_pattern_p (false, operands[0])" > + { return umips_output_save_restore (false, operands[0]); } > + [(set_attr "type" "multimem") > + (set_attr "mode" "SI") > + (set_attr "can_delay" "no")]) ;; The behaviour of LWM is undefined if placed in a delay slot. > +; For lwp > +(define_peephole2 ;; For LWP. (Two semicolons, period at the end.) > + [(set (match_operand:SI 0 "d_operand" "") > + (match_operand:SI 1 "non_volatile_mem_operand" "")) > + (set (match_operand:SI 2 "d_operand" "") > + (match_operand:SI 3 "non_volatile_mem_operand" ""))] > + "TARGET_MICROMIPS > + && umips_load_store_pair_p (true, operands)" > + [(parallel [(set (match_dup 0) (match_dup 1)) > + (set (match_dup 2) (match_dup 3))])] > +) > + > +;; The behavior of the lwp insn is undefined if placed in a delay slot. Single space after ";;". > +; For swp ;; For SWP. > +(define_insn "*swp" ;; The behaviour of SWP is undefined if placed in a delay slot. > +; For movep ;; For MOVEP. > +;; The behavior of the movep insn is undefined if placed in a delay slot. Single space after ";;" rather than two. > +(define_memory_constraint "ZC" > + "When compiling microMIPS code, this constraing matches a memory operand s/constraing/constraint/ > + whose address is formed from a base register and a 12-bit offset. These > + operands can be used for microMIPS instructions such as @code{ll} and > + @code{sc}. When not compiling for microMIPS code, @code{ZC} is > + equivalent to @code{R}." > + (and (match_code "mem") > + (ior (and (match_test "TARGET_MICROMIPS") > + (match_test "umips_12bit_offset_address_p (XEXP (op, 0), mode)")) > + (match_test "mips_address_insns (XEXP (op, 0), mode, false)")))) > + > +(define_address_constraint "ZD" > + "When compiling microMIPS code, this constraint matches an address operand > + that is formed from a base register and a 12-bit offset. These operands > + can be used for microMIPS instructions such as @code{prefetch}. When > + not compiling for microMIPS code, @code{YC} is equivalent to @code{p}." s/YC/ZD/ > +(define_predicate "movep_src_register" > + (and (match_code "reg") > + (ior (match_test ("IN_RANGE (REGNO (op), 0, 3)")) > + (match_test ("IN_RANGE (REGNO (op), 16, 20)"))))) s/0, 3/2, 3/. $1 isn't valid, and $0 should always be represented as zero in RTL. > @@ -451,10 +458,17 @@ > ;; from the shorten_branches reference address. > (and (eq_attr "type" "branch") > (not (match_test "TARGET_MIPS16"))) > - (cond [(and (le (minus (match_dup 0) (pc)) (const_int 131064)) > - (le (minus (pc) (match_dup 0)) (const_int 131068))) > + ;; any variant can handle the 17-bit range > + (cond [(and (le (minus (match_dup 0) (pc)) (const_int 65532)) > + (le (minus (pc) (match_dup 0)) (const_int 65534))) > (const_int 4) Formatting: (cond [;; Any variant can handle the 17-bit range. (and (le (minus (match_dup 0) (pc)) (const_int 65532)) (le (minus (pc) (match_dup 0)) (const_int 65534))) (const_int 4) > + ;; The 18-bit range is OK other than for microMIPS. > + (and (eq (symbol_ref "TARGET_MICROMIPS") (const_int 0)) (not (match_test "TARGET_MICROMIPS")) > @@ -5445,12 +5462,35 @@ > (match_operand:GPR 3 "reg_or_0_operand" "dJ")]) > (label_ref (match_operand 0 "" "")) > (pc)))] > - "!TARGET_MIPS16" > + "!TARGET_MIPS16 && !TARGET_MICROMIPS" > { > return mips_output_conditional_branch (insn, operands, > MIPS_BRANCH ("b%C1", "%2,%z3,%0"), > MIPS_BRANCH ("b%N1", "%2,%z3,%0")); > } > + [(set_attr "type" "branch") > + (set_attr "mode" "none")]) > + > +(define_insn "*branch_equality_micromips" > + [(set (pc) > + (if_then_else > + (match_operator 1 "equality_operator" > + [(match_operand:GPR 2 "register_operand" "d") > + (match_operand:GPR 3 "reg_or_0_operand" "dJ")]) > + (label_ref (match_operand 0 "" "")) > + (pc)))] > + "TARGET_MICROMIPS" > +{ > + /* For a simple bnez or beqz microMIPS branch. */ > + if (operands[3] == const0_rtx && get_attr_length (insn) <= 8) > + return mips_output_conditional_branch (insn, operands, > + "%*b%C1z%:\t%2,%0", > + "%*b%N1z%:\t%2,%0"); > + > + return mips_output_conditional_branch (insn, operands, > + MIPS_BRANCH ("b%C1", "%2,%z3,%0"), > + MIPS_BRANCH ("b%N1", "%2,%z3,%0")); > +} The pattern and the fallback case look the same as for !TARGET_MICRORMIPS. I think we should use the existing *branch_equality for microMIPS and just add: /* For a simple BNEZ or BEQZ microMIPS branch. */ if (TARGET_MICROMIPS && operands[3] == const0_rtx && get_attr_length (insn) <= 8) return mips_output_conditional_branch (insn, operands, "%*b%C1z%:\t%2,%0", "%*b%N1z%:\t%2,%0"); > @@ -5461,7 +5501,7 @@ > (match_operand:GPR 3 "reg_or_0_operand" "dJ")]) > (pc) > (label_ref (match_operand 0 "" ""))))] > - "!TARGET_MIPS16" > + "!TARGET_MIPS16 && !TARGET_MICROMIPS" > { > return mips_output_conditional_branch (insn, operands, > MIPS_BRANCH ("b%N1", "%2,%z3,%0"), > @@ -5532,6 +5572,29 @@ > (label_ref (match_operand 1)) > (pc)))]) > > +(define_insn "*branch_equality_inverted_micromips" > + [(set (pc) > + (if_then_else > + (match_operator 0 "equality_operator" > + [(match_operand:GPR 2 "register_operand" "d") > + (match_operand:GPR 3 "reg_or_0_operand" "dJ")]) > + (pc) > + (label_ref (match_operand 1 "" ""))))] > + "TARGET_MICROMIPS" > +{ > + /* For a simple bnez or beqz microMIPS branch. */ > + if (operands[3] == const0_rtx && get_attr_length (insn) <= 8) > + return mips_output_conditional_branch (insn, operands, > + "%*b%N0z%:\t%2,%1", > + "%*b%C0z%:\t%2,%1"); > + > + return mips_output_conditional_branch (insn, operands, > + MIPS_BRANCH ("b%N0", "%2,%z3,%1"), > + MIPS_BRANCH ("b%C0", "%2,%z3,%1")); > +} > + [(set_attr "type" "branch") > + (set_attr "mode" "none")]) > + Same comment here. > @@ -6350,7 +6435,12 @@ > [(call (mem:SI (match_operand 0 "call_insn_operand" "j,S")) > (match_operand 1 "" ""))] > "TARGET_SIBCALLS && SIBLING_CALL_P (insn)" > - { return MIPS_CALL ("j", operands, 0, 1); } > + { > + if (TARGET_MICROMIPS) > + return MICROMIPS_J ("j", operands, 0); > + else > + return MIPS_CALL ("j", operands, 0, 1); > + } Please put the multiline { ... } block in column 0: { if (TARGET_MICROMIPS) return MICROMIPS_J ("j", operands, 0); else return MIPS_CALL ("j", operands, 0, 1); } (You did this in the bits I snipped, thanks.) > @@ -6371,7 +6461,12 @@ > (call (mem:SI (match_operand 1 "call_insn_operand" "j,S")) > (match_operand 2 "" "")))] > "TARGET_SIBCALLS && SIBLING_CALL_P (insn)" > - { return MIPS_CALL ("j", operands, 1, 2); } > + { > + if (TARGET_MICROMIPS) > + return MICROMIPS_J ("j", operands, 1); > + else > + return MIPS_CALL ("j", operands, 1, 2); > + } > [(set_attr "jal" "indirect,direct") > (set_attr "jal_macro" "no")]) Same here. > @@ -6383,7 +6478,12 @@ > (call (mem:SI (match_dup 1)) > (match_dup 2)))] > "TARGET_SIBCALLS && SIBLING_CALL_P (insn)" > - { return MIPS_CALL ("j", operands, 1, 2); } > + { > + if (TARGET_MICROMIPS) > + return MICROMIPS_J ("j", operands, 1); > + else > + return MIPS_CALL ("j", operands, 1, 2); > + } Same here. > Index: config/mips/mips.c > =================================================================== > --- config/mips/mips.c (revision 195900) > +++ config/mips/mips.c (working copy) > @@ -77,6 +77,9 @@ > preserve the maximum stack alignment. We therefore use a value > of 0x7ff0 in this case. > + microMIPS LWM and SWM support 12-bit offsets (from -0x800 to 0x7ff), > + so we use a maximum of 0x7f0 for TARGET_MICROMIPS. > + > MIPS16e SAVE and RESTORE instructions can adjust the stack pointer by > up to 0x7f8 bytes and can usually save or restore all the registers > that we need to save or restore. (Note that we can only use these > @@ -86,8 +89,10 @@ > RESTORE are not available. We can then use unextended instructions > to save and restore registers, and to allocate and deallocate the top > part of the frame. */ > + No extra whitespace. > +/* Return the attribute name associated with MASK_MIPS16 and MASK_MICROMIPS > + flags FLAGS. */ Too much indentation: /* Return the attribute name associated with MASK_MIPS16 and MASK_MICROMIPS flags FLAGS. */ > +/* Return the set of compression modes that are explicitly required > + by DECL. */ > + > +static unsigned int > +mips_get_compress_on_flags (tree decl) > { > + unsigned int flags = 0; > + > + if (lookup_attribute ("mips16", DECL_ATTRIBUTES (decl)) != NULL) > + flags |= MASK_MIPS16; > + > + if (lookup_attribute ("micromips", DECL_ATTRIBUTES (decl)) != NULL) > + flags |= MASK_MICROMIPS; > + > + return flags; > +} > + > +/* Return the set of compression modes that are explicitly forbidden > + by DECL. */ > + > +static unsigned int > +mips_get_compress_off_flags (tree decl) > +{ > + unsigned int flags = 0; > + > + if (lookup_attribute ("nocompression", DECL_ATTRIBUTES (decl)) != NULL) > + flags |= MASK_MIPS16 | MASK_MICROMIPS; > + > + if (lookup_attribute ("nomips16", DECL_ATTRIBUTES (decl)) != NULL) > + flags |= MASK_MIPS16; > + > + if (lookup_attribute ("nomicromips", DECL_ATTRIBUTES (decl)) != NULL) > + flags |= MASK_MICROMIPS; > + > + return flags; > +} This and... > +/* Set compression flags based on attributes. */ > + > +static unsigned int > +mips_get_compression_attrs (tree *attributes) > +{ > + unsigned int compression_flags = 0; > + if (lookup_attribute ("mips16", *attributes) != NULL) > + compression_flags |= MASK_MIPS16; > + if (lookup_attribute ("micromips", *attributes) != NULL) > + compression_flags |= MASK_MICROMIPS; > + return compression_flags; > +} > + > +/* Set nocompression flags based on attributes. */ > + > +static unsigned int > +mips_get_nocompression_attrs (tree *attributes) > +{ > + unsigned int nocompression_flags = 0; > + > + if (lookup_attribute ("nomips16", *attributes) != NULL) > + nocompression_flags |= MASK_MIPS16; > + if (lookup_attribute ("nomicromips", *attributes) != NULL) > + nocompression_flags |= MASK_MICROMIPS; > + if (lookup_attribute ("nocompression", *attributes) != NULL) > + nocompression_flags |= MASK_MICROMIPS | MASK_MIPS16; > + return nocompression_flags; > +} ...this are really duplicates of each other. Let's just have one. /* Return the set of compression modes that are explicitly required by ATTRIBUTES. */ static unsigned int mips_get_compress_on_flags (tree attributes) { unsigned int flags = 0; if (lookup_attribute ("mips16", attributes) != NULL) flags |= MASK_MIPS16; if (lookup_attribute ("micromips", attributes) != NULL) flags |= MASK_MICROMIPS; return flags; } /* Return the set of compression modes that are explicitly forbidden by ATTRIBUTES. */ static unsigned int mips_get_compress_off_flags (tree attributes) { unsigned int flags = 0; if (lookup_attribute ("nocompression", attributes) != NULL) flags |= MASK_MIPS16 | MASK_MICROMIPS; if (lookup_attribute ("nomips16", attributes) != NULL) flags |= MASK_MIPS16; if (lookup_attribute ("nomicromips", attributes) != NULL) flags |= MASK_MICROMIPS; return flags; } Then pass DECL_ATTRIBUTES (decl) when we have a decl. > + /* If DECL is "nocompression" set the "nomips16" and > + "nomicromips" attributes. */ > + if (nocompression_flags & MASK_MIPS16 > + && nocompression_flags & MASK_MICROMIPS) > { > - /* DECL cannot be simultaneously "mips16" and "nomips16". */ > - if (mips16_p && nomips16_p) > - error ("%qE cannot have both % and " > - "% attributes", > - DECL_NAME (decl)); > + name = "nomips16"; > + *attributes = tree_cons (get_identifier (name), NULL, *attributes); > + name = "nomicromips"; > + *attributes = tree_cons (get_identifier (name), NULL, *attributes); > } As above, I don't think we want this. > - else if (TARGET_FLIP_MIPS16 && !DECL_ARTIFICIAL (decl)) > + else if (TARGET_FLIP_MIPS16 && !DECL_ARTIFICIAL (decl)) The original indentation was correct. > { > /* Implement -mflip-mips16. If DECL has neither a "nomips16" nor a > "mips16" attribute, arbitrarily pick one. We must pick the same > setting for duplicate declarations of a function. */ > name = mflip_mips16_use_mips16_p (decl) ? "mips16" : "nomips16"; > + name = "nomips16"; > + *attributes = tree_cons (get_identifier (name), NULL, *attributes); > + name = "nomicromips"; > *attributes = tree_cons (get_identifier (name), NULL, *attributes); > - } > + } This looks wrong; it unconditionally overrides the first "name" assignment. The point is that we want to alternately add "nomips16" and "mips16" attributes to functions that have neither. I think this should be: if (TARGET_FLIP_MIPS16 && !DECL_ARTIFICIAL (decl) && compression_flags == 0 && nocompression_flags == 0) { ...existing block... } > +/* Return true if X is a legitimate address with a 12-bit offset. > + MODE is the mode of the value being accessed. */ > + > +bool > +umips_12bit_offset_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 > + && CONST_INT_P (addr.offset) > + && UMIPS_12BIT_OFFSET_P (addr.offset)); As mentioned above, this is missing an INTVAL: && UMIPS_12BIT_OFFSET_P (INTVAL (addr.offset))); > +/* Return true if a call to DECL may need to use JALX. */ > + > +static bool > +mips_call_may_need_jalx_p (tree decl) > +{ > + /* If the current translation unit would use a different mode for DECL, > + assume that the call needs JALX. */ > + if (mips_get_compress_mode (decl) != TARGET_COMPRESSION) > + return true; > + > + /* mips_get_compress_mode is always accurate for locally-binding > + functions in the current translation unit. */ > + if (!DECL_EXTERNAL (decl) && targetm.binds_local_p (decl)) > + return false; > + > + /* PLTs use the standard encoding, so calls that might go via PLTs > + must use JALX. */ > + if (TARGET_COMPRESSION > + && TARGET_ABICALLS_PIC0 > + && DECL_EXTERNAL (decl) > + && !targetm.binds_local_p (decl)) > + return true; As before, please leave the PLT block out; we'll rely on Maciej's linker patch instead. > /* Implement TARGET_FUNCTION_OK_FOR_SIBCALL. */ > static bool > mips_function_ok_for_sibcall (tree decl, tree exp ATTRIBUTE_UNUSED) > { > + > + unsigned int compression_mode = mips_get_compress_mode (decl); No blank line here. > @@ -6916,22 +7095,43 @@ > if (mips_interrupt_type_p (TREE_TYPE (current_function_decl))) > return false; > + if (TARGET_MICROMIPS) > + { > + > + /* We can't do a sibcall if the called function isn't microMIPS. */ > + if (decl > + && (compression_mode & MASK_MICROMIPS) == 0 > + && const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode)) > + return false; > + > + /* Direct Js are only possible to functions that use the same ISA > + encoding. There is no JX counterpoart of JALX. */ > + if (decl > + && const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode) > + && mips_call_may_need_jalx_p (decl)) > + return false; > + > + /* Otherwise OK. */ > + return true; > + } > + > > /* We can't do a sibcall if the called function is a MIPS16 function > because there is no direct "jx" instruction equivalent to "jalx" to > switch the ISA mode. We only care about cases where the sibling > and normal calls would both be direct. */ > if (decl > - && mips_use_mips16_mode_p (decl) > + && ((compression_mode & (MASK_MIPS16 | MASK_MICROMIPS)) != 0) > && const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode)) > return false; > - /* When -minterlink-mips16 is in effect, assume that non-locally-binding > - functions could be MIPS16 ones unless an attribute explicitly tells > + /* When -minterlink-compressed is in effect, assume that non-locally-binding > + functions could be MIPS16 or microMIPS unless an attribute explicitly tells > us otherwise. */ > - if (TARGET_INTERLINK_MIPS16 > + if (TARGET_INTERLINK_COMPRESSED > && decl > && (DECL_EXTERNAL (decl) || !targetm.binds_local_p (decl)) > - && !mips_nomips16_decl_p (decl) > + && (compression_mode & MASK_MICROMIPS) == 0 > + && (compression_mode & MASK_MIPS16) == 0 > && const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode)) > return false; The idea was that all this ("if (TARGET_MICROMIPS)" onwards) would be replaced by a call to mips_call_may_need_jalx_p. I.e.: if (decl && mips_call_may_need_jalx_p (decl) && const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode)) return false; > +/* Save register REG to MEM. Make the instruction frame-related. */ > + > +static void > +mips_save_reg (rtx reg, rtx mem) > +{ > + if (GET_MODE (reg) == DFmode && !TARGET_FLOAT64) > + { > + rtx x1, x2; > + > + mips_emit_move_or_split (mem, reg, SPLIT_IF_NECESSARY); > + > + x1 = mips_frame_set (mips_subword (mem, false), > + mips_subword (reg, false)); > + x2 = mips_frame_set (mips_subword (mem, true), > + mips_subword (reg, true)); > + mips_set_frame_expr (gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, x1, x2))); > + } > + else > + mips_emit_save_slot_move (mem, reg, MIPS_PROLOGUE_TEMP (GET_MODE (reg))); > +} > + > + Just one blank line between functions. > +/* Capture the register combinations that are allowed in a swm or lwm > + instruction. The entries are ordered by number of registers set in > + the mask. We also ignore the single register encodings because a > + normal sw/lw is preferred. */ As above, please use caps for instruction names. > + > +static const unsigned int umips_swm_mask[17] = { > + 0xc0ff0000, 0x80ff0000, 0x40ff0000, 0x807f0000, > + 0x00ff0000, 0x803f0000, 0x007f0000, 0x801f0000, > + 0x003f0000, 0x800f0000, 0x001f0000, 0x80070000, > + 0x000f0000, 0x80030000, 0x00070000, 0x80010000, > + 0x00030000 }; > + > +static const unsigned int umips_swm_encoding[17] = { > + 25, 24, 9, 23, 8, 22, 7, 21, 6, 20, 5, 19, 4, 18, 3, 17, 2 }; > + > + Indent the conents of { ... } by two spaces rather than one. "};" should go on its own line. Just one blank line after "umips_swm_encoding". > +/* Try to use a microMIPS LWM or SWM instruction to save or restore > + as many GPRs in *MASK as possible. *OFFSET is the offset from the > + stack pointer of the topmost save slot. > + > + Remove from *MASK all registers that were handled using LWM and SWM. > + Update *OFFSET so that it points to the first unused save slot. */ > + > +static bool > +umips_build_save_restore (mips_save_restore_fn fn, > + unsigned *mask, HOST_WIDE_INT *offset) > +{ > + int i, nregs; > + unsigned int j; > + rtx pattern, set, reg, mem; > + HOST_WIDE_INT this_offset; > + rtx this_base; > + > + /* Try matching $16 to $31 (s0 to ra). */ > + for (i = 0; i < ARRAY_SIZE (umips_swm_mask); i++) > + if ((*mask & 0xffff0000) == umips_swm_mask[i]) > + break; > + > + if (i == ARRAY_SIZE (umips_swm_mask)) > + return false; > + > + /* Adjust offset for output. */ > + nregs = (umips_swm_encoding[i] & 0xf) + (umips_swm_encoding[i] >> 4); > + *offset -= (UNITS_PER_WORD * (nregs - 1)); > + > + /* LWM/SWM can only support offsets from -2048 to 2047. */ > + if (!UMIPS_12BIT_OFFSET_P (*offset)) > + return false; This changes "*offset" even on a false return. Please use: /* Work out the offset of the first register. */ nregs = (umips_swm_encoding[i] & 0xf) + (umips_swm_encoding[i] >> 4); this_offset = *offset - UNITS_PER_WORD * (nregs - 1); /* LWM/SWM can only support offsets from -2048 to 2047. */ if (!UMIPS_12BIT_OFFSET_P (this_offset)) return false; > + /* Create the final PARALLEL. */ > + pattern = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (nregs)); > + > + this_base = stack_pointer_rtx; > + this_offset = *offset; > + > + /* For registers $16-$23 and $30. */ > + for (j = 0; j < (umips_swm_encoding[i] & 0xf); j++) > + { > + unsigned int regno; > + HOST_WIDE_INT offset = this_offset + j * UNITS_PER_WORD; > + mem = gen_frame_mem (SImode, plus_constant (Pmode, this_base, offset)); > + regno = (j != 8) ? 16 + j : 30; > + *mask &= 0 << regno; *mask &= ~(1 << regno); ("*mask &= 0 << regno" sets *mask to 0). > + reg = gen_rtx_REG (SImode, regno); > + if (fn == mips_save_reg) > + set = mips_frame_set (mem, reg); > + else > + { > + set = gen_rtx_SET (VOIDmode, reg, mem); > + mips_add_cfa_restore (reg); > + } > + XVECEXP (pattern, 0, j) = set; > + } > + > + /* For register $31. */ > + if (umips_swm_encoding[i] >> 4) > + { > + long long offset = this_offset + j * UNITS_PER_WORD; > + *mask &= 0 << 31; Same here. Also, HOST_WIDE_INT rather than "long long". > + mem = gen_frame_mem (SImode, plus_constant (Pmode, this_base, offset)); > + reg = gen_rtx_REG (SImode, 31); > + if (fn == mips_save_reg) > + set = mips_frame_set (mem, reg); > + else > + { > + set = gen_rtx_SET (VOIDmode, reg, mem); > + mips_add_cfa_restore (reg); > + } > + XVECEXP (pattern, 0, j) = set; > + } > + > + pattern = emit_insn (pattern); > + if (fn == mips_save_reg) > + RTX_FRAME_RELATED_P (pattern) = 1; > + > + /* Adjust the last offset. */ > + offset -= UNITS_PER_WORD; After the change above, this becaomes: *offset -= UNITS_PER_WORD * nregs; > @@ -10245,16 +10585,21 @@ > mips_save_restore_fn fn) > { > enum machine_mode fpr_mode; > - HOST_WIDE_INT offset; > int regno; > + const struct mips_frame_info *frame = &cfun->machine->frame; > + HOST_WIDE_INT offset = frame->gp_sp_offset - sp_offset; > + unsigned int mask = frame->mask; > /* Save registers starting from high to low. The debuggers prefer at least > the return register be stored at func+4, and also it allows us not to > need a nop in the epilogue if at least one register is reloaded in > addition to return address. */ > - offset = cfun->machine->frame.gp_sp_offset - sp_offset; Please keep this as it is and set up "mask" here too. Now that we're C++, there's no need to move everything to the head of the function. > @@ -16233,7 +16560,7 @@ > align_jumps = mips_base_align_jumps; > align_functions = mips_base_align_functions; > - if (mips16_p) > + if (compression_mode & MASK_MIPS16) > { > /* Switch to MIPS16 mode. */ > target_flags |= MASK_MIPS16; > @@ -16288,8 +16615,15 @@ > else > { > /* Switch to normal (non-MIPS16) mode. */ > - target_flags &= ~MASK_MIPS16; > + target_flags &= ~(MASK_MIPS16 | MASK_MICROMIPS); > + target_flags |= compression_mode; This part: target_flags &= ~(MASK_MIPS16 | MASK_MICROMIPS); target_flags |= compression_mode; should go before: if (compression_mode & MASK_MIPS16) replacing both: target_flags |= MASK_MIPS16; and: target_flags &= ~MASK_MIPS16; "Switch to normal (non-MIPS16) mode." should become "Switch to microMIPS or the standard encoding." > + if (TARGET_MICROMIPS) > + { > + /* Avoid branch likely. */ > + target_flags &= ~MASK_BRANCHLIKELY; > + } Redundant { ... } > static void > mips_set_current_function (tree fndecl) > { > - mips_set_mips16_mode (mips_use_mips16_mode_p (fndecl)); > + There shouldn't be a blank line here. > + unsigned int compression_p = mips_get_compress_mode (fndecl); > + > + mips_set_compression_mode (compression_p & (MASK_MIPS16 | MASK_MICROMIPS)); This should just be: mips_set_compression_mode (mips_get_compress_mode (fndecl)); > - Do all CPP-sensitive stuff in non-MIPS16 mode; we'll switch to > - MIPS16 mode afterwards if need be. */ > - mips_set_mips16_mode (false); > + Do all CPP-sensitive stuff in non-MIPS16/non-microMIPS mode; "uncompressed" rather than "non-MIPS16/non-microMIPS". > + We'll switch modes later if required. */ > + mips_set_compression_mode (false); 0 rather than false. > + > + > +/* Return true if PATTERN matches the kind of instruction generated by > + umips_build_save_restore. SAVE_P is true for store. */ Only one blank line between functions. > + > +bool > +umips_save_restore_pattern_p (bool save_p, rtx pattern) > +{ > + int n; > + unsigned int i; > + HOST_WIDE_INT first_offset = 0; > + rtx first_base = 0; > + unsigned int regmask = 0; > + > + for (n = 0; n < XVECLEN (pattern, 0); n++) > + { > + rtx set, reg, mem, this_base; > + HOST_WIDE_INT this_offset; > + unsigned int this_regno; > + > + /* Check that we have a SET. */ > + set = XVECEXP (pattern, 0, n); > + if (GET_CODE (set) != SET) > + return false; > + > + /* Check that the SET is a load (if restoring) or a store > + (if saving). */ > + mem = save_p ? SET_DEST (set) : SET_SRC (set); > + if (!MEM_P (mem) || MEM_VOLATILE_P (mem)) > + return false; > + > + /* Check that the address is the sum of base and a possibly-zero > + constant offset. Determine if the offset is in range. */ > + mips_split_plus (XEXP (mem, 0), &this_base, &this_offset); > + if (!REG_P (this_base)) > + return false; > + > + if (n == 0) > + { > + if (!UMIPS_12BIT_OFFSET_P (this_offset)) > + return false; > + first_base = this_base; > + first_offset = this_offset; > + } > + else > + { > + /* Check that the save slots are consecutive. */ > + if (REGNO (this_base) != REGNO (first_base) > + || this_offset != first_offset + UNITS_PER_WORD * n) > + return false; > + } > + > + /* Check that SET's other operand is a register. */ > + reg = save_p ? SET_SRC (set) : SET_DEST (set); > + if (!REG_P (reg)) > + return false; > + > + this_regno = REGNO (reg); > + regmask |= 1 << this_regno; I'd prefer just: regmask |= 1 << REGNO (reg); I don't think the this_regno temporary buys anything. > +/* Return true if MEM1 and MEM2 use the same base register, and the > + offset of MEM2 equals the offset of MEM1 plus 4. FIRST_REG is the > + register into (from) which the contents of MEM1 will be loaded > + (stored), depending on the value of LOAD_P. > + SWAP_P is true when the 1st and 2nd instructions are swapped. */ > + > +static bool > +umips_load_store_pair_p_1 (bool load_p, bool swap_p, rtx first_reg, > + rtx mem1, rtx mem2) > +{ > + rtx base1, base2; > + HOST_WIDE_INT offset1, offset2; > + > + if (!MEM_P (mem1) || !MEM_P (mem2)) > + return false; > + > + mips_split_plus (XEXP (mem1, 0), &base1, &offset1); > + mips_split_plus (XEXP (mem2, 0), &base2, &offset2); > + > + if (!REG_P (base1) || !rtx_equal_p (base1, base2)) > + return false; > + > + /* Avoid invalid load pair instructions. */ > + if (load_p && REGNO (first_reg) == REGNO (base1)) > + return false; > + > + /* We must avoid this case for anti-dependence. > + Ex: lw $3, 4($3) > + lw $2, 0($3) > + first_reg is $2, but the base is $3. */ > + if (load_p > + && swap_p > + && REGNO (first_reg) + 1 == REGNO (base1)) > + return false; > + > + if (swap_p > + && offset2 + 4 != offset1) > + return false; > + > + if (!swap_p > + && offset1 + 4 != offset2) > + return false; > + > + if (!UMIPS_12BIT_OFFSET_P (offset1)) > + return false; Looks like this tests the wrong offset in the swap_p case. offset2 is the one that will be used in the instruction. I think it would be easier to swap MEM1 and MEM2 around when calling this function with SWAP_P true. Then the last three conditions become: if (offset2 != offset1 + 4) return false; if (!UMIPS_12BIT_OFFSET_P (offset1)) return false; This also makes the function behave in the way that the comment above says (it says that MEM2 must be 4 bytes after MEM1). I.e swap mem1 and mem2... > + if (REGNO (reg1) == REGNO (reg2) + 1) > + return umips_load_store_pair_p_1 (load_p, true, reg2, mem1, mem2); ...here. > +/* Return true if reg1 and reg2 match the criteria for a movep insn. */ > + > +bool > +umips_movep_target_p (rtx reg1, rtx reg2) REG1 and REG2 (caps) in the comment. > +{ > + int regno1, regno2, pair; > + unsigned int i; > + static const int match[8] = { > + 0x00000060, /* 5, 6 */ > + 0x000000a0, /* 5, 7 */ > + 0x000000c0, /* 6, 7 */ > + 0x00200010, /* 4, 21 */ > + 0x00400010, /* 4, 22 */ > + 0x00000030, /* 4, 5 */ > + 0x00000050, /* 4, 6 */ > + 0x00000090 /* 4, 7 */ > + }; Two spaces of indentation in match[8] = { ... }; > @@ -17342,7 +17959,7 @@ > It therefore seems best to switch back to non-MIPS16 mode at > save time, and to ensure that mips16_globals remains null after > a PCH load. */ > - mips_set_mips16_mode (false); > + mips_set_compression_mode (false); 0 rather than false. > Index: Makefile.in > =================================================================== > --- Makefile.in (revision 195931) > +++ Makefile.in (working copy) > @@ -223,7 +223,7 @@ > # Options to use when compiling libgcc2.a. > # > LIBGCC2_DEBUG_CFLAGS = -g > -LIBGCC2_CFLAGS = -O2 $(LIBGCC2_INCLUDES) $(GCC_CFLAGS) $(HOST_LIBGCC2_CFLAGS) \ > +LIBGCC2_CFLAGS = $(LIBGCC2_INCLUDES) $(GCC_CFLAGS) $(HOST_LIBGCC2_CFLAGS) \ > $(LIBGCC2_DEBUG_CFLAGS) -DIN_LIBGCC2 \ > -fbuilding-libgcc -fno-stack-protector \ > $(INHIBIT_LIBC_CFLAGS) Looks like this snuck in accidentally. Please make sure that the patch works without it. :-) > Index: gcc.target/mips/umips-lwp-swp-1.c > =================================================================== > --- gcc.target/mips/umips-lwp-swp-1.c (revision 0) > +++ gcc.target/mips/umips-lwp-swp-1.c (revision 0) > @@ -0,0 +1,19 @@ > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" "-O1" } { "" } } */ > + > +void MICROMIPS > +foo (long long l1, long long *l2) > +{ > + *l2 = l1; > +} > + > +long long MICROMIPS > +bar (long long d) > +{ > + register long long l1 asm ("$8") = d; > + register long long l2 asm ("$12") = 0; > + asm ("#foo" : "=r" (l1) : "r" (l1)); > + asm volatile ("#foo" :: "r" (l2)); > + return l1; > +} > + > +/* { dg-final { scan-assembler "\tswp" } */ I don't understand what bar() does here. It looks like the SWP comes from foo() alone. Using SWP for "long long" requires a 32-bit target, so please add: /* { dg-options "-mgp32 (-mmicromips)" } */ I notice you're changing mips-sde-elf; that'd be a good target to make sure that the testsuite is still 64-bit clean. E.g. one run with -mips64r2 (and without -mmicromips). Also, please add specific registers to the scan-assembler line. The SWP in foo() should always be "swp $4,0($6)". > Index: gcc.target/mips/umips-save-restore-1.c > =================================================================== > --- gcc.target/mips/umips-save-restore-1.c (revision 0) > +++ gcc.target/mips/umips-save-restore-1.c (revision 0) > +/* Check that we can use the swm/lwm instructions. */ > +/* { dg-options "(-mmicromips)" } */ > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */ > + > +#include > + > +int bar (int, int, int, int, int); > + > +MICROMIPS int > +foo (int n, int a, int b, int c, int d) > +{ > + int i, j; > + > + i = bar (n, a, b, c, d); > + j = bar (n, a, b, c, d); > + return i + j; > +} > +/* { dg-final { scan-assembler "\tswm\t" } } */ > +/* { dg-final { scan-assembler "\tlwm\t" } } */ No need for stdarg.h. Again I think the scan-assembler should have specific registers and offsets, since all the interesting code is to do with picking ranges and offsets. It's easier to check for specific offsets if we fix an ABI: /* { dg-options "-mabi=32 (-mmicromips)" } */ > Index: gcc.target/mips/umips-lwp-swp-2.c > =================================================================== > --- gcc.target/mips/umips-lwp-swp-2.c (revision 0) > +++ gcc.target/mips/umips-lwp-swp-2.c (revision 0) > @@ -0,0 +1,19 @@ > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" "-O1" } { "" } } */ > + > +void MICROMIPS > +foo (long long *l1, long long l2) > +{ > + *l1 = l2; > +} > + > +long long MICROMIPS > +bar (long long d) > +{ > + register long long l1 asm ("$8") = d; > + register long long l2 asm ("$12") = 0; > + asm ("#foo" : "=r" (l1) : "r" (l1)); > + asm volatile ("#foo" :: "r" (l2)); > + return l1; > +} > + > +/* { dg-final { scan-assembler "\tswp" } */ Same comments as for umips-lwp-swp-1.c. All this really does is swap the arguments to foo() around, so that instead of: swp $4, 0($6) we have: swp $6, 0($4) I certainly don't mind having both, but they really test the same code path. What we don't have is a test for the swap_p case. I think we need one, in addition to one or both of the above. > Index: gcc.target/mips/umips-save-restore-2.c > =================================================================== > --- gcc.target/mips/umips-save-restore-2.c (revision 0) > +++ gcc.target/mips/umips-save-restore-2.c (revision 0) > @@ -0,0 +1,15 @@ > +/* Check that we can use the save instruction to save spilled arguments. */ > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */ > + > +MICROMIPS void > +foo (int *a, int b, int c) > +{ > + asm volatile ("" ::: "$2", "$3", "$4", "$5", "$6", "$7", "$8", > + "$9", "$10", "$11", "$12", "$13", "$14", "$15", "$16", > + "$17", "$18", "$19", "$20", "$21", "$22", "$23", "$24", > + "$25", "$30", "memory"); > + a[b] = 1; > + a[c] = 1; > +} > +/* { dg-final { scan-assembler "\tswm\t" } } */ > +/* { dg-final { scan-assembler "\tlwm\t" } } */ Same comments as for umips-save-restore-1.c. > Index: gcc.target/mips/umips-lwp-swp-volatile.c > =================================================================== > --- gcc.target/mips/umips-lwp-swp-volatile.c (revision 0) > +++ gcc.target/mips/umips-lwp-swp-volatile.c (revision 0) > @@ -0,0 +1,41 @@ > +/* { dg-do compile } */ > + > +/* This test ensures that we do not generate microMIPS SWP or LWP > + instructions when any component of the accessed memory is volatile; > + they are unsafe for such since they might cause replay of partial > + accesses if interrupted by an exception. */ > + > +static void set_csr (volatile void *p, int v) > +{ > + *(volatile int *) (p) = v; > +} > + > +static int get_csr (volatile void *p) > +{ > + return *(volatile int *) (p); > +} > + > +int main () > +{ > + int i, q = 0, p = 0, r = 0; > + > + for (i = 0; i < 20; i++) > + { > + set_csr ((volatile void *) 0xbf0100a8, 0xffff0002); > + set_csr ((volatile void *) 0xbf0100a4, 0x80000008); > + } > + > + for (i = 0; i < 20; i++) > + { > + register int k, j; > + k = get_csr ((volatile void *) 0xbf0100b8); > + p += k; > + j = get_csr ((volatile void *) 0xbf0100b4); > + r += j; > + q = j + k; > + } > + return q + r + p; > +} > + > +/* { dg-final { scan-assembler-not "\tswp\t\\\$" } } */ > +/* { dg-final { scan-assembler-not "\tlwp\t\\\$" } } */ Just "\tswp" and "\tlwp" for scan-assembler-not. Longer strings run the risk that the regexps will accidentally miss SWP and LWP instructions because of incorrect backslashes, etc. > Index: gcc.target/mips/umips-save-restore-3.c > =================================================================== > --- gcc.target/mips/umips-save-restore-3.c (revision 0) > +++ gcc.target/mips/umips-save-restore-3.c (revision 0) > @@ -0,0 +1,13 @@ > +/* Check that we can use the swm instruction to save $16, $17 and $31. */ > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */ > + > +void bar (void); > + > +MICROMIPS void > +foo (void) > +{ > + bar (); > + asm volatile ("" ::: "$16", "$17"); > +} > +/* { dg-final { scan-assembler "\tswm\t\\\$16-\\\$17,\\\$31" } } */ > +/* { dg-final { scan-assembler "\tlwm\t\\\$16-\\\$17,\\\$31" } } */ Looks good, thanks, but it needs: /* { dg-options "-mgp32 (-mmicromips)" } */ > Index: gcc.target/mips/umips-movep.c > =================================================================== > --- gcc.target/mips/umips-movep.c (revision 0) > +++ gcc.target/mips/umips-movep.c (revision 0) > @@ -0,0 +1,18 @@ > +/* Check that we can use the save instruction to save varargs. */ > +/* { dg-options "(-mmicromips)" } */ > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" "-O1" } { "" } } */ > + > +#include > + > +long long bar (long long, long long); > + > +MICROMIPS long long > +foo (long long n, long long a) > +{ > + long long i, j; > + > + i = bar (n, a); > + j = bar (n, a); > + return i + j; > +} > +/* { dg-final { scan-assembler "\tmovep\t" } } */ The comment at the head of the file doesn't seem to match the test. No need for . If I understand correctly: int bar (int, int); MICROMIPS foo (int a, int b) { return bar (0, 0); } ought to be able to use MOVEP too, is that right? A test for this kind of zero case would be good. Richard