From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3836 invoked by alias); 22 Jan 2013 20:23:55 -0000 Received: (qmail 3805 invoked by uid 22791); 22 Jan 2013 20:23:49 -0000 X-SWARE-Spam-Status: No, hits=-3.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_EQ X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 22 Jan 2013 20:23:35 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1TxkNZ-0002xT-H1 from Catherine_Moore@mentor.com ; Tue, 22 Jan 2013 12:23:33 -0800 Received: from SVR-ORW-FEM-03.mgc.mentorg.com ([147.34.97.39]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Tue, 22 Jan 2013 12:23:33 -0800 Received: from NA-MBX-01.mgc.mentorg.com ([169.254.1.207]) by SVR-ORW-FEM-03.mgc.mentorg.com ([147.34.97.39]) with mapi id 14.01.0289.001; Tue, 22 Jan 2013 12:23:32 -0800 From: "Moore, Catherine" To: Richard Sandiford CC: "gcc-patches@gcc.gnu.org" , "Rozycki, Maciej" , "Moore, Catherine" Subject: RE: FW: [PATCH] [MIPS] microMIPS gcc support Date: Tue, 22 Jan 2013 20:23:00 -0000 Message-ID: References: <87y5mfjm4c.fsf@talisman.home> In-Reply-To: <87y5mfjm4c.fsf@talisman.home> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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-01/txt/msg01117.txt.bz2 Hi Richard, I hope that I have addressed all of your comments in this cleaned up patch.= There are a couple of items that have been omitted that I plan to submit = as follow-on patches. The use of jraddiusp and the use of short delay slot= s have both been deferred. You had suggested a function named mips_get_com= pression_name but I did not see where this was used. It's been omitted for= now. I cleaned up most of the gcc.target/mips tests for microMIPS, but s= ome of the MIPS16-specific tests will fail. It looks like an overhaul of m= ips.exp may be required and that is beyond the scope of this patch. Let me know what you think. I'll submit the optimizations once the base pa= tch is approved. Thanks, Catherine > -----Original Message----- > From: Richard Sandiford [mailto:rdsandiford@googlemail.com] > Sent: Thursday, July 19, 2012 8:46 PM > To: Moore, Catherine > Cc: gcc-patches@gcc.gnu.org > Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support >=20 > "Moore, Catherine" writes: > > Forgot to copy the list ... >=20 > Same with my reply... >=20 > > Here is the updated microMIPS patch. It's been a very long time (two > > years!) since I posted the original. Please let me know what we're > > going to need to do to get this committed. >=20 > This is looking better. At least that huge if statement in > mips_adjust_insn_length (IIRC) is gone :-) >=20 > ------------------------------------------------------------------------- > @itemx -mno-interlink-mips16 > @opindex minterlink-mips16 > @opindex mno-interlink-mips16 > -Require (do not require) that non-MIPS16 code be link-compatible with > -MIPS16 code. > +Require (do not require) that non-MIPS16/non-microMIPS code be > +link-compatible with MIPS16/microMIPS code. >=20 > -For example, non-MIPS16 code cannot jump directly to MIPS16 code; > +For example, non-MIPS16/non-microMIPS code cannot jump directly to > +MIPS16/microMIPS code; > it must either use a call or an indirect jump. @option{-minterlink-mips= 16} > therefore disables direct jumps unless GCC knows that the target of the - > jump is not MIPS16. > +jump is not MIPS16/non microMIPS. > ------------------------------------------------------------------------- >=20 > This doesn't read very well. Let's add a -minterlink-compressed option a= nd > treat -mno-interlink-mips16 as an alias of it. So: >=20 > ------------------------------------------------------------------------- > @item -minterlink-compressed > @itemx -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. >=20 > For example, non-MIPS16 code cannot jump directly to MIPS16 code; it must > either use a call or an indirect jump. The same applies to non-microMIPS > jumps to microMIPS code. @option{-minterlink-compressed} therefore > disables direct jumps unless GCC knows that the target of the jump is not > compressed. >=20 > @item -minterlink-mips16 > @itemx -mno-interlink-mips16 > @opindex minterlink-mips16 > @opindex mno-interlink-mips16 > Aliases of @option{-minterlink-compressed} and @option{-mno-interlink- > compressed}. These options predate the mipsMIPS ASE and are retained for > backwards compatiblity. > ------------------------------------------------------------------------- >=20 > Let's also make TARGET_INTERLINK_COMPRESSED the internal name. >=20 > ------------------------------------------------------------------------- > +@item -mmicromips > +@itemx -mno-micromips > +@opindex mmicromips > +@opindex mno-mmicromips > +Generate (do not generate) microMIPS code. If GCC is targetting a > +MIPS32 or MIPS64 architecture, it will make use of the microMIPS ASE@. > ------------------------------------------------------------------------- >=20 > Looks like excess cut-&-paste from the MIPS16 version. The point of the > MIPS16 documentation is to distinguish between the original MIPS16 "ASE" > and MIPS16e. There's no such distinction here. >=20 > ------------------------------------------------------------------------- > +@item -mjals > +@itemx -mno-jals > +@opindex mjals > +@opindex mno-jals > +Generate (do not generate) the @code{jals} instruction for microMIPS by > +recognizing that the branch delay slot instruction can be 16 bits. > +This implies that the funciton call cannot switch the current mode > +during the linking stage, because we don't have the @code{jalxs} > +instruction that supports 16-bit branch delay slot instructions. > ------------------------------------------------------------------------- >=20 > typo: function >=20 > The assumption we're making seems to be that calls from microMIPS code > cannot go to non-microMIPS code. If so, let's make that the option inste= ad. > The above is too low-level. >=20 > With analogy to -minterlink-compressed, the option could be -minterlink- > uncompressed/-mno-interlink-uncompressed, with the default being - > minterlink-uncompressed. >=20 > ------------------------------------------------------------------------- > +@item YC > +For MIPS, it is the same as the constraint @code{R}. For microMIPS, it > +matches an address within a 12-bit offset that can be used for > +microMIPS @code{ll}, @code{sc}, etc. > + > +@item YD > +For MIPS, it is the same as the constraint @code{p} For microMIPS, it > +matches a 12-bit offsest address. > + > +@item YE > +A singler register memory operand. > ------------------------------------------------------------------------- >=20 > "For MIPS" is a bit vague (it actually applies to MIPS16 too). > So how about: >=20 > ------------------------------------------------------------------------- > @item YC > when compiling microMIPS code, this constraint matches a memory operand > 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{YC} is > equivalent to @code{R}. >=20 > @item YD > 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}. > ------------------------------------------------------------------------- >=20 > No need for "YE": we have "ZR" now. I'd like to make the other constrain= ts > "Z" rather than "Y" too -- ZC and ZD -- since "Y" is really for constants. >=20 > ------------------------------------------------------------------------- > +(define_insn "*store_word_multiple" > + [(match_parallel 0 "" > + [(set (match_operand:SI 1 "memory_operand") > + (match_operand:SI 2 "register_operand"))])] > + "TARGET_MICROMIPS > + && umips_save_restore_pattern_p (true, operands[0])" > + { > + return umips_output_save_restore (true, operands[0]); > + } > + [(set_attr "type" "multimem") > + (set_attr "mode" "SI") > + (set_attr "can_delay" "no")]) > ------------------------------------------------------------------------- >=20 > Nitlet: local style is to use "{ return ...; }" for single-line statement= s and > column-0 placement of "{" and "}" for multi-line ones. Same for rest of = file. >=20 > ------------------------------------------------------------------------- > +; For lwp > +(define_peephole2 > + [(set (match_operand:SI 0 "umips_lwp_register" "") > + (match_operand:SI 1 "non_volatile_mem_operand" "")) > + (set (match_operand:SI 2 "umips_lwp_register" "") > + (match_operand:SI 3 "non_volatile_mem_operand" ""))] > + "TARGET_MICROMIPS > + && ((REGNO (operands[0]) + 1 =3D=3D REGNO (operands[2]) > + && umips_load_store_pair_p (true, false, operands[0], > + operands[1], operands[3])) > + || (REGNO (operands[2]) + 1 =3D=3D REGNO (operands[0]) > + && umips_load_store_pair_p (true, true, operands[2], > + operands[3], operands[1])))" > + [(parallel [(set (match_dup 0) (match_dup 1)) > + (set (match_dup 2) (match_dup 3))])] > ------------------------------------------------------------------------- >=20 > Please split out this: >=20 > && ((REGNO (operands[0]) + 1 =3D=3D REGNO (operands[2]) > && umips_load_store_pair_p (true, false, operands[0], > operands[1], operands[3])) > || (REGNO (operands[2]) + 1 =3D=3D REGNO (operands[0]) > && umips_load_store_pair_p (true, true, operands[2], > operands[3], operands[1]))) >=20 > into a function: >=20 > bool > umips_load_store_pair_p (bool load_p, rtx reg1, rtx reg2, rtx mem1, rtx > mem2) >=20 > with the current umips_load_store_pair_p being a subroutine > (umips_load_store_pair_p_1). >=20 > ------------------------------------------------------------------------- > +(define_insn "*lwp" > + [(parallel [(set (match_operand:SI 0 "umips_lwp_register") > + (match_operand:SI 1 "memory_operand")) > + (set (match_operand:SI 2 "umips_lwp_register") > + (match_operand:SI 3 "memory_operand"))])] > + > + "TARGET_MICROMIPS > + && ((REGNO (operands[0]) + 1 =3D=3D REGNO (operands[2]) > + && umips_load_store_pair_p (true, false, operands[0], > + operands[1], operands[3])) > + || (REGNO (operands[2]) + 1 =3D=3D REGNO (operands[0]) > + && umips_load_store_pair_p (true, true, operands[2], > + operands[3], operands[1])))" > + { > + if (REGNO (operands[0]) + 1 =3D=3D REGNO (operands[2])) > + return umips_output_load_store_pair (true, operands[0], operands[1= ]); > + else > + return umips_output_load_store_pair (true, operands[2], > +operands[3]); > + } > + [(set_attr "type" "load") > + (set_attr "mode" "SI") > + (set_attr "can_delay" "no")]) > ------------------------------------------------------------------------- >=20 > Same with the output: >=20 > void > umips_output_load_store_pair (bool load_p, rtx reg1, rtx reg2, > rtx mem1, rtx mem2) >=20 > Why do only the stores require non_volatile_mem_operand? It looks like > you could be reordering loads from volatile memory otherwise. >=20 > ------------------------------------------------------------------------- > +; For jraddiusp > +(define_insn "mips_jraddiusp" > + [(parallel [(return) > + (use (reg:SI 31)) > + (set (reg:SI 29) > + (plus:SI (reg:SI 29) > + (match_operand 0 "const_int_operand")))])] > + "TARGET_MICROMIPS" > + "jraddiusp\t%0" > + [(set_attr "type" "trap") > + (set_attr "mode" "SI") > + (set_attr "can_delay" "no")]) > ------------------------------------------------------------------------- >=20 > Redundant comment. >=20 > ------------------------------------------------------------------------- > +; For movep > +(define_peephole2 > + [(set (match_operand:SI 0 "register_operand" "") > + (match_operand:SI 1 "movep_si_operand" "")) > + (set (match_operand:SI 2 "register_operand" "") > + (match_operand:SI 3 "movep_si_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))])] > +) > + > +(define_peephole2 > + [(set (match_operand:SI 0 "register_operand" "") > + (match_operand:SI 1 "movep_si_operand" "")) > + (set (match_operand:SF 2 "register_operand" "") > + (match_operand:SF 3 "const_0_sf_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))])] > +) > + > +(define_peephole2 > + [(set (match_operand:SF 0 "register_operand" "") > + (match_operand:SF 1 "const_0_sf_operand" "")) > + (set (match_operand:SI 2 "register_operand" "") > + (match_operand:SI 3 "movep_si_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))])] > +) > + > +(define_peephole2 > + [(set (match_operand:SF 0 "register_operand" "") > + (match_operand:SF 1 "const_0_sf_operand" "")) > + (set (match_operand:SF 2 "register_operand" "") > + (match_operand:SF 3 "const_0_sf_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))])] > +) > + > +(define_insn "*movep" > + [(parallel [(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])" > +{ > + if (REGNO (operands[0]) < REGNO (operands[2])) > + return "movep\t%0,%2,%z1,%z3"; > + else > + return "movep\t%2,%0,%z3,%z1"; > +} > + [(set_attr "type" "move") > + (set_attr "mode" "") > + (set_attr "can_delay" "no")]) > ------------------------------------------------------------------------- >=20 > Seems to be a mismatch between the peepholes (which only accept 0.0f for > SFmode) and the insn (which allows registers too). Can't you use MOVEP1 > and MOVEP2 for the peepholes too? >=20 > ------------------------------------------------------------------------- > (define_memory_constraint "R" > "An address that can be used in a non-macro load or store." > (and (match_code "mem") > - (match_test "mips_address_insns (XEXP (op, 0), mode, false) =3D= =3D 1"))) > + (match_test "mips_address_insns (XEXP (op, 0), mode, false)"))) > ------------------------------------------------------------------------- >=20 > No, this defeats the purpose of "R", which is supposed to disallow address > that require assembly macros. >=20 > ------------------------------------------------------------------------- > +(define_memory_constraint "YC" > + "For MIPS, it is the same as the constraint R. For microMIPS, it matc= hes > + an address within a 12-bit offset that can be used in ll, sc, etc." > + (and (match_code "mem") > + (ior (and (match_test "TARGET_MICROMIPS") > + (match_test "umips_address_insns (XEXP (op, 0), mode, > false)")) > + (match_test "mips_address_insns (XEXP (op, 0), mode, false)")))) > + > +(define_address_constraint "YD" > + "@internal > + For MIPS, it is the same as the constraint p. For microMIPS, it matc= hes > + a 12-bit offset address." > + (ior (and (match_test "TARGET_MICROMIPS") > + (match_test "umips_address_insns (op, mode, false)")) > + (match_test "mips_address_insns (op, mode, false)"))) > ------------------------------------------------------------------------- >=20 > Is YD internal or not? We should remove it from the docs if so, otherwis= e the > @internal is wrong. >=20 > Please keep the doc strings exactly the same as the .texi version (includ= ing @ > mark-up). I think the idea was that at some glorious time in the future,= we'd > be able to auto-generate the .texi parts from the .md doc strings. >=20 > ------------------------------------------------------------------------- > +(define_predicate "gpr_operand" > + (and (match_code "reg") > + (match_test "GP_REG_P (REGNO (op))"))) > ------------------------------------------------------------------------- >=20 > This is d_operand. >=20 > ------------------------------------------------------------------------- > +(define_predicate "umips_register" > + (and (match_code "reg") > + (match_test "M16_REG_P (REGNO (op))"))) > ------------------------------------------------------------------------- >=20 > I think I'd prefer to call the predicate m16_register too, for consistenc= y. >=20 > ------------------------------------------------------------------------- > +(define_predicate "umips_andi16_operand" > + (and (match_code "const") > + (match_test "UMIPS_ANDI16_IMM (INTVAL (op))"))) > ------------------------------------------------------------------------- >=20 > Looks like this should be "const_int" instead. Same for the other consta= nts. > It seems on face value like some parts of the patch weren't exercised > because of this. >=20 > ------------------------------------------------------------------------- > +(define_predicate "umips_add_uimm5" > + (and (match_code "const") > + (match_test "IN_RANGE (INTVAL (op), -8, 7)"))) > ------------------------------------------------------------------------- >=20 > Looks like a signed rather than unsigned immediate. Should this be > "umips_add_imm5" instead? >=20 > ------------------------------------------------------------------------- > +(define_predicate "umips_addiur2_imm3" > + (and (match_code "const") > + (ior (match_test "INTVAL(op) =3D=3D 1") > + (ior (match_test "INTVAL(op) =3D=3D 4") > + (ior (match_test "INTVAL(op) =3D=3D 8") > + (ior (match_test "INTVAL(op) =3D=3D 12") > + (ior (match_test "INTVAL(op) =3D=3D 16") > + (ior (match_test "INTVAL(op) =3D=3D 20") > + (ior (match_test "INTVAL(op) =3D=3D 24") > + (match_test "INTVAL(op) =3D=3D -1")))))))))) > ------------------------------------------------------------------------- >=20 > (ior ...) can take any number of operands these days, so no need for all = those > ")"s. Space after INTVAL. >=20 > ------------------------------------------------------------------------- > +(define_predicate "umips_lw16_memref" > + (and (match_code "plus") > + (match_operand 0 "umips_register" "")) { > + rtx op2 =3D XEXP (op, 1); > + > + return (GET_CODE (op2) =3D=3D CONST_INT > + && ((INTVAL (op2) & 3) =3D=3D 0) > + && (IN_RANGE (INTVAL (op2), 0, 60))); > +}) > ------------------------------------------------------------------------- >=20 > This tests an address, whereas it sounds from the name (and looks from the > later code) like it's supposed to be testing a (mem ...) instead. > Also: >=20 > (and (match_code "plus") > (match_operand 0 "umips_register" "")) >=20 > tests for something that is both a PLUS and a REG, which is never true. > ((match_operand X ...) doesn't test XEXP (..., X).) >=20 > ------------------------------------------------------------------------- > +(define_predicate "const_0_si_operand" > + (and (match_code "const_int,const_double,const_vector") > + (match_test "op =3D=3D CONST0_RTX (SImode)"))) > + > +(define_predicate "const_0_sf_operand" > + (and (match_code "const_int,const_double,const_vector") > + (match_test "op =3D=3D CONST0_RTX (SFmode)"))) > ------------------------------------------------------------------------- >=20 > Should be redundant with const_0_operand. Modes should be checked at > the match_operand level where important. >=20 > ------------------------------------------------------------------------- > +(define_predicate "movep_si_operand" > + (ior (and (match_code "const_int,const_double,const_vector") > + (match_test "op =3D=3D CONST0_RTX (SImode)")) > + (and (match_code "reg") > + (ior (match_test ("IN_RANGE (REGNO (op), 0, 3)")) > + (match_test ("IN_RANGE (REGNO (op), 16, 20)")))))) > + > +(define_predicate "movep_sf_operand" > + (ior (and (match_code "const_int, const_double, const_vector") > + (match_test "op =3D=3D CONST0_RTX (SFmode)")) > + (and (match_code "reg") > + (ior (match_test ("IN_RANGE (REGNO (op), 0, 3)")) > + (match_test ("IN_RANGE (REGNO (op), 16, 20)")))))) > ------------------------------------------------------------------------- >=20 > These two should be combined: >=20 > ------------------------------------------------------------------------- > (define_predicate "movep_register" > (and (match_code "reg") > (ior (match_test ("IN_RANGE (REGNO (op), 0, 3)")) > (match_test ("IN_RANGE (REGNO (op), 16, 20)"))))) >=20 > (define_predicate "movep_operand" > (ior (match_operand 0 "const_0_operand") > (match_operand 0 "movep_register"))) > ------------------------------------------------------------------------- >=20 > ------------------------------------------------------------------------- > +(define_predicate "umips_lwp_register" > + (and (match_code "reg") > + (match_test "REGNO (op) <=3D GP_REG_LAST"))) > ------------------------------------------------------------------------- >=20 > Reundant with d_operand/gpr_operand? >=20 > ------------------------------------------------------------------------- > +;; Return 1 if the operand is in non-volatile memory. Note that during > +the ;; RTL generation phase, memory_operand does not return TRUE for > +volatile ;; memory references. So this function allows us to recognize > +volatile ;; references where it's safe. > +(define_predicate "non_volatile_mem_operand" > + (not (and (and (match_code "mem") > + (match_test "MEM_VOLATILE_P (op)")) > + (if_then_else (match_test "reload_completed") > + (match_operand 0 "memory_operand") > + (if_then_else (match_test "reload_in_progress") > + (match_test "strict_memory_address_p (mode, XEXP (op, 0))") > + (match_test "memory_address_p (mode, XEXP (op, 0))")))))) > ------------------------------------------------------------------------- >=20 > This looks wrong: the whole thing is wrapped in (not ...), so it matches > something that specifically _isn't_ a memory_operand. All that reload st= uff > shouldn't be needed, plain: >=20 > (define_predicate "non_volatile_mem_operand" > (and (match_operand 0 "memory_operand") > (not (match_test "MEM_VOLATILE_P (op)")))) >=20 > should be OK. >=20 > ------------------------------------------------------------------------- > +(define_attr "umips_not" "no, yes" > + (if_then_else (and (eq_attr "alu_type" "not") > + (match_test "umips_two_reg (PATTERN (insn))")) > + (const_string "yes") > + (const_string "no"))) > ------------------------------------------------------------------------- >=20 > Looks odd: reg-to-reg is the only possiblity for "not". > Should be enough to test "alu_type" against "not" directly and drop this > attribute. >=20 > ------------------------------------------------------------------------- > +(define_attr "umips_arith" "no, yes" > + (if_then_else (and (eq_attr "type" "arith") > + (not (eq_attr "mode" "DI")) > + (match_test "umips_three_reg_match0 (PATTERN > (insn))")) > + (const_string "yes") > + (const_string "no"))) > ------------------------------------------------------------------------- >=20 > Well, I suppose this OK for now, but it seems odd to be hiding the differ= ence > from the main patterns. Don't we want IRA+reload to optimise for this > where possible? Or does that produce bad results? > More below. >=20 > Rather than define lots of attributes, please just use [(cond ...)] to se= t the > default directly: >=20 > (define_attr "umips_short_insn" "no, yes" > (cond [(and (eq_attr "type" "arith") > (not (eq_attr "mode" "DI")) > (match_test "umips_three_reg_match0 (PATTERN (insn))")) > (const_string "yes") >=20 > ...] > (const_string "no"))) >=20 > ------------------------------------------------------------------------- > +(define_attr "umips_mfhi" "no, yes" > + (if_then_else (and (eq_attr "move_type" "mfhilo") > + (match_operand 1 "gpr_operand" "")) > + (const_string "yes") > + (const_string "no"))) > ------------------------------------------------------------------------- >=20 > Patch collision: as of a couple of days ago, we now have an "mfhi" > "type" attribute. >=20 > ------------------------------------------------------------------------- > +(define_attr "umips_logicals" "no, yes" > + (if_then_else (and (ior (eq_attr "alu_type" "and") > + (eq_attr "alu_type" "or") > + (eq_attr "alu_type" "xor")) > + (match_test "umips_three_reg_match0 (PATTERN > (insn))")) > + (const_string "yes") > + (const_string "no"))) > ------------------------------------------------------------------------- >=20 > (eq_attr "alu_type" "and,or,xor"). >=20 > ------------------------------------------------------------------------- > ;; Direct branch instructions have a range of [-0x20000,0x1fffc], > - ;; relative to the address of the delay slot. If a branch is > - ;; outside this range, we have a choice of two sequences. > + ;; relative to the address of the delay slot. > + ;; > + ;; For microMIPS the range is reduced to [-0x10000,0xfffe]. > + ;; > + ;; If a branch is outside this range, we have a choice of two > + ;; sequences. > ------------------------------------------------------------------------- >=20 > I'd prefer: > ------------------------------------------------------------------------- > ;; Direct microMIPS branch instructions have a range of > ;; [-0x10000,0xfffe], otherwise the range is [-0x20000,0x1fffc]. > ;; If a branch is outside this range, we have a choice of two > ;; sequences. > ------------------------------------------------------------------------- >=20 > ------------------------------------------------------------------------- > ;; pattern has no explicit delay slot, mips_adjust_insn_length > ;; will add the length of the implicit nop. The values for > ;; forward and backward branches will be different as well. > + > ------------------------------------------------------------------------- >=20 > Excess whitespace. >=20 > ------------------------------------------------------------------------- > + (eq_attr "type" "multimem") > + (const_int 4) > ------------------------------------------------------------------------- >=20 > Not necessary, 4 is the default. >=20 > ------------------------------------------------------------------------- > +(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 (!TARGET_BRANCHLIKELY > + && get_attr_length (insn) <=3D 8 > + && GET_CODE (operands[3]) =3D=3D CONST_INT > + && INTVAL (operands[3]) =3D=3D 0) > + 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")); } > ------------------------------------------------------------------------- >=20 > This: >=20 > && GET_CODE (operands[3]) =3D=3D CONST_INT > && INTVAL (operands[3]) =3D=3D 0) >=20 > can just be: >=20 > && operands[3] =3D=3D const0_rtx) >=20 > Why the !TARGET_BRANCHLIKELY test? You force it off anyway in > mips_set_mips16_micromips_mode (which I agree is a good thing) >=20 > ------------------------------------------------------------------------- > (define_insn "indirect_jump_" > [(set (pc) (match_operand:P 0 "register_operand" "d"))] > "" > - "%*j\t%0%/" > +{ > + if (TARGET_MICROMIPS) > + return "%*jr%:\t%0"; > + else > + return "%*j\t%0%/"; > +} > ------------------------------------------------------------------------- >=20 > "jr" should be fine across the board. Same later. >=20 > ------------------------------------------------------------------------- > + microMIPS LWM and SWM support 12-bit offsets (from -2048 to 2047) and > + to preserve the maximum stack alignment, so 0x7f0 is used when > + TARGET_MICROMIPS. > ------------------------------------------------------------------------- >=20 > I'd prefer: >=20 > ------------------------------------------------------------------------- > microMIPS LWM and SWM support 12-bit offsets (from -0x800 to 0x7ff), > so we use a maximum of 0x7f0 for TARGET_MICROMIPS. > ------------------------------------------------------------------------- >=20 > (where the context already describes the alignment constraint). >=20 > ------------------------------------------------------------------------- > +static bool > +mips_nocompression_decl_p (const_tree decl) > +{ > + if (lookup_attribute ("nocompression", DECL_ATTRIBUTES (decl)) !=3D NU= LL) > + return true; > + > + return (lookup_attribute ("nomips16", DECL_ATTRIBUTES (decl)) !=3D NULL > + && lookup_attribute ("nomicromips", DECL_ATTRIBUTES (decl)) != =3D > NULL); > +} > ... > +/* Similar predicates for "micromips"/"nomicromips" function attributes.= */ > + > +static bool > +mips_micromips_decl_p (const_tree decl) > +{ > + return lookup_attribute ("micromips", DECL_ATTRIBUTES (decl)) !=3D NUL= L; > +} > + > +static bool > +mips_nomicromips_decl_p (const_tree decl) > +{ > + return lookup_attribute ("nomicromips", DECL_ATTRIBUTES (decl)) !=3D > NULL; > +} > + > +/* Return true if function DECL is a microMIPS function. Return the amb= ient > + setting if DECL is null. */ > + > +static bool > +mips_use_micromips_mode_p (tree decl) > +{ > + if (decl) > + { > + /* Nested functions must use the same frame pointer as their > + parent and must therefore use the same ISA mode. */ > + tree parent =3D decl_function_context (decl); > + if (parent) > + decl =3D parent; > + if (mips_micromips_decl_p (decl)) > + return true; > + if (mips_nomicromips_decl_p (decl)) > + return false; > + } > + return mips_base_micromips; > +} > ------------------------------------------------------------------------- >=20 > To reduce the cut-&-paste here and elsewhere, I think it would be > better to have: >=20 > ------------------------------------------------------------------------- > unsigned int mips_base_compression_flags; > ------------------------------------------------------------------------- >=20 > to replace mips_base_mips16 & mips_base_micromips and: >=20 > ------------------------------------------------------------------------- > /* Return the set of compression modes that are explicitly required > by DECL. */ >=20 > static unsigned int > mips_get_compress_on_flags (tree decl) > { > unsigned int flags =3D 0; >=20 > if (lookup_attribute ("mips16", DECL_ATTRIBUTES (decl)) !=3D NULL) > flags |=3D MASK_MIPS16; >=20 > if (lookup_attribute ("micromips", DECL_ATTRIBUTES (decl)) !=3D NULL) > flags |=3D MASK_MICROMIPS; >=20 > return flags; > } >=20 > /* Return the set of compression modes that are explicitly forbidden > by DECL. */ >=20 > static unsigned int > mips_get_compress_off_flags (tree decl) > { > unsigned int flags =3D 0; >=20 > if (lookup_attribute ("nocompression", DECL_ATTRIBUTES (decl)) !=3D NUL= L) > flags |=3D MASK_MIPS16 | MASK_MICROMIPS; >=20 > if (lookup_attribute ("nomips16", DECL_ATTRIBUTES (decl)) !=3D NULL) > flags |=3D MASK_MIPS16; >=20 > if (lookup_attribute ("nomicromips", DECL_ATTRIBUTES (decl)) !=3D NULL) > flags |=3D MASK_MICROMIPS; >=20 > return flags; > } >=20 > /* Return the compression mode that should be used for function DECL. > Return the ambient setting if DECL is null. */ >=20 > static unsigned int > mips_get_compress_mode (tree decl) > { > unsigned int flags, force_on; >=20 > flags =3D mips_base_compression_flags; > if (decl) > { > /* Nested functions must use the same frame pointer as their > parent and must therefore use the same ISA mode. */ > tree parent =3D decl_function_context (decl); > if (parent) > decl =3D parent; > force_on =3D mips_get_compression_on_flags (decl); > if (force_on) > return force_on; > flags &=3D ~mips_get_compression_off_flags (decl); > } > return flags; > } >=20 > /* Return the name of the compression mode represented by target > flags FLAGS. */ >=20 > static const char * > mips_get_compression_name (unsigned int flags) > { > if (flags & TARGET_MIPS16) > return "mips16"; > if (flags & TARGET_MICROMIPS) > return "micromips"; > gcc_unreachable(); > } > ------------------------------------------------------------------------- >=20 > then use these instead of tests for individual mips16 or micromips > attributes. >=20 > ------------------------------------------------------------------------- > switch (addr.type) > { > case ADDRESS_REG: > + > if (TARGET_MIPS16 > && !mips16_unextended_reference_p (mode, addr.reg, > UINTVAL (addr.offset))) > ------------------------------------------------------------------------- >=20 > Excess whitespace. >=20 > ------------------------------------------------------------------------- > +int > +umips_address_insns (rtx x, enum machine_mode mode, bool > might_split_p) > +{ > + struct mips_address_info addr; > + int factor; > + > + /* BLKmode is used for single unaligned loads and stores and should > + not count as a multiword mode. (GET_MODE_SIZE (BLKmode) is pretty > + meaningless, so we have to single it out as a special case one way > + or the other.) */ > + if (mode !=3D BLKmode && might_split_p) > + factor =3D (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / > UNITS_PER_WORD; > + else > + factor =3D 1; > + > + if (mips_classify_address (&addr, x, mode, false)) > + switch (addr.type) > + { > + case ADDRESS_REG: > + if (!CONST_INT_P (addr.offset) > + || INTVAL (addr.offset) < -2048 > + || INTVAL (addr.offset) > 2047) > + return 0; > + > + return factor; > + > + default: > + break; > + > + } > + return 0; > +} > ------------------------------------------------------------------------- >=20 > Function lacks a comment. It's only use is for YC/YD, which always > pass false for might_split_p, so this is really just a predicate > that tests for 12-bit offset addresses. So: >=20 > ------------------------------------------------------------------------- > /* Return true if X is a legimate address with a 12-bit offset. > MODE is the mode of the value being accessed. */ >=20 > bool > umips_12bit_offset_address_p (rtx x, enum machine_mode mode) > { > struct mips_address_info addr; >=20 > return (mips_classify_address (&addr, x, mode, false) > && addr.type =3D=3D ADDRESS_REG > && CONST_INT_P (addr.offset) > && IN_RANGE (INTVAL (addr.offset), -2048, 2047)); > } > ------------------------------------------------------------------------- >=20 > Seems like you could reuse this elsewhere in places that also > check for 12-bit offsets. >=20 > ------------------------------------------------------------------------- > +/* Return true if the insn has three micromips register operands. */ > +int > +umips_three_reg (rtx insn) > +{ > + rtx op0 =3D XEXP (insn, 0); > + rtx op1 =3D XEXP (insn, 1); > + rtx op2 =3D XEXP (insn, 2); > + > + return (op0 !=3D NULL_RTX > + && op1 !=3D NULL_RTX > + && op2 !=3D NULL_RTX > + && REG_P (op0) > + && M16_REG_P (REGNO (op0)) > + && REG_P (op1) > + && M16_REG_P (REGNO (op1)) > + && REG_P (op2) > + && M16_REG_P (REGNO (op2))); > +} > ------------------------------------------------------------------------- >=20 > This doesn't look right. You pass in a complete PATTERN, > so XEXP (insn, 1) will typically be the SET_SRC (such as a PLUS) > and XEXP (insn, 2) usually isn't valid. >=20 > I think --enable-checking=3Dyes,rtl would have tripped over the op2 > extraction and flagged it as a problem. Could you use that option > when testing the updated patch? >=20 > Same problem with the other predicates. >=20 > Please make the predicates return bool rather than int. >=20 > Like I said above, I'd really prefer to see the register requirements > modelled in the patterns directly. You could use the "enabled" to > prevent the new alternatives from being used by non-MIPS16 code. >=20 > ------------------------------------------------------------------------- > + if (TARGET_MICROMIPS) > + fprintf (asm_out_file, "\t.set\tmicromips\n"); > + else > + fprintf (asm_out_file, "\t.set\tnomicromips\n"); > ------------------------------------------------------------------------- >=20 > We need a configure test to see whether the assembler supports > microMIPS. > We shouldn't emit ".set\tnomicromips" if it doesn't. >=20 > ------------------------------------------------------------------------- > + /* When reorder or noreorder with final_squence 0, the delay slot = will > + be a nop, so we just use the compact version for microMIPS. */ > + if (mips_noreorder.nesting_level =3D=3D 0 || final_sequence =3D=3D= 0) > + putc ('c', file); > ------------------------------------------------------------------------- >=20 > Should just be "final_sequence =3D=3D 0". Same for '%!'. >=20 > ------------------------------------------------------------------------- > +static void mips_save_reg (rtx reg, rtx mem); > ------------------------------------------------------------------------- >=20 > Please move the functions around so that this isn't necessary. >=20 > ------------------------------------------------------------------------- > +/* Build microMIPS save or restore. FN is save or restore function. > + OFFSET is the current stack offset. > + Return true if we succeed creating save or restore. */ > ------------------------------------------------------------------------- >=20 > /* Consider building a microMIPS LDM or STM for the current stack frame. > FN is mips_save_reg for stores or mips_restore_reg for loads. > OFFSET is the offset of the first register save slot from the > current stack pointer. >=20 > Return true on success, in which case all GPRs will have been saved. = */ >=20 > ------------------------------------------------------------------------- > +static bool > +umips_build_save_restore (mips_save_restore_fn fn, > + HOST_WIDE_INT offset) > ------------------------------------------------------------------------- >=20 > Odd formatting. >=20 > ------------------------------------------------------------------------- > + unsigned int type[19] =3D {0x00010000, 0x00030000, 0x00070000, 0x000f0= 000, > + 0x001f0000, 0x003f0000, 0x007f0000, 0x00ff0000, > + 0x40ff0000, 0x80000000, 0x80010000, 0x80030000, > + 0x80070000, 0x800f0000, 0x801f0000, 0x803f0000, > + 0x807f0000, 0x80ff0000, 0xc0ff0000}; > + unsigned int encode[19] =3D {1, 2, 3, 4, 5, 6, 7, 8, 9, 16, 17, 18, 19, > + 20, 21, 22, 23, 24, 25}; > ------------------------------------------------------------------------- >=20 > Make these static const arrays. >=20 > ------------------------------------------------------------------------- > + /* Try matching $16 to $31 (s0 to ra). */ > + for (i =3D 0; i < 19; i++) > + if ((cfun->machine->frame.mask & 0xffff0000) =3D=3D type[i]) > + break; > ------------------------------------------------------------------------- >=20 > ARRAY_SIZE (type) instead of 19 (here and elsewhere). >=20 > ------------------------------------------------------------------------- > + /* For only one register, we use normal sw/lw for speed. */ > + if (i =3D=3D 0 || i =3D=3D 9) > + return false; > ------------------------------------------------------------------------- >=20 > Seems easier to drop these two elements from the array (with a comment). >=20 > ------------------------------------------------------------------------- > + /* For $31 to $24. */ > + if (i < 8 && (cfun->machine->frame.mask & 0xff000000)) > + { > + int regno; > + for (regno =3D GP_REG_LAST; regno > GP_REG_LAST - 8; regno--) > + if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST)) > + { > + mips_save_restore_reg (word_mode, regno, offset, fn); > + offset -=3D UNITS_PER_WORD; > + } > + } > ------------------------------------------------------------------------- >=20 > Rather than this, please just use: >=20 > ------------------------------------------------------------------------- > extra_regs =3D (cfun->machine->frame.mask & ~type[i]); > for (regno =3D GP_REG_LAST; regno > GP_REG_LAST - 8; regno--) > if (BITSET_P (extra_regs, regno - GP_REG_FIRST)) > { > mips_save_restore_reg (word_mode, regno, offset, fn); > offset -=3D UNITS_PER_WORD; > } > ------------------------------------------------------------------------- >=20 > ------------------------------------------------------------------------- > + mem =3D gen_frame_mem (SImode, plus_constant (Pmode, this_base, > + this_offset + j * UNITS_PER_WORD)); > ------------------------------------------------------------------------- >=20 > Nonstandard formatting. Might be easiest to split out the offset calcula= tion. >=20 > ------------------------------------------------------------------------- > @@ -10050,6 +10434,13 @@ mips_for_each_saved_gpr_and_fpr > (HOST_WIDE_INT sp_ > need a nop in the epilogue if at least one register is reloaded in > addition to return address. */ > offset =3D cfun->machine->frame.gp_sp_offset - sp_offset; > + > + if (TARGET_MICROMIPS) > + { > + if (umips_build_save_restore (fn, offset)) > + goto save_restore_fp_reg; > + } > + > for (regno =3D GP_REG_LAST; regno >=3D GP_REG_FIRST; regno--) > if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST)) > { > @@ -10060,6 +10451,8 @@ mips_for_each_saved_gpr_and_fpr > (HOST_WIDE_INT sp_ > offset -=3D UNITS_PER_WORD; > } >=20 > +save_restore_fp_reg: > + > /* This loop must iterate over the same space as its companion in > mips_compute_frame_info. */ > offset =3D cfun->machine->frame.fp_sp_offset - sp_offset; > ------------------------------------------------------------------------- >=20 > No!!! :-) Just wrap the GPR save in: >=20 > if (!(TARGET_MICROMIPS && umips_build_save_restore (fn, offset))) >=20 > Please also split out the save loop, since we now have three copies of it. >=20 > ------------------------------------------------------------------------- > static void > -mips_deallocate_stack (rtx base, rtx offset, HOST_WIDE_INT > new_frame_size) > +mips_deallocate_stack (rtx base, rtx offset, HOST_WIDE_INT > new_frame_size, > + bool use_jraddiusp_p) > { > + if (use_jraddiusp_p) > + return; > + > if (base =3D=3D stack_pointer_rtx && offset =3D=3D const0_rtx) > return; >=20 > mips_frame_barrier (); > + > ------------------------------------------------------------------------- >=20 > Seems odd to add a parameter that causes the function to return instantly. > But... >=20 > ------------------------------------------------------------------------- > const struct mips_frame_info *frame; > HOST_WIDE_INT step1, step2; > rtx base, adjust, insn; > + bool use_jraddiusp_p =3D false; >=20 > if (!sibcall_p && mips_can_use_return_insn ()) > { > @@ -10831,7 +11230,7 @@ mips_expand_epilogue (bool sibcall_p) > mips_emit_move (MIPS_EPILOGUE_TEMP (Pmode), adjust); > adjust =3D MIPS_EPILOGUE_TEMP (Pmode); > } > - mips_deallocate_stack (base, adjust, step2); > + mips_deallocate_stack (base, adjust, step2, use_jraddiusp_p); >=20 > /* If we're using addressing macros, $gp is implicitly used by all > SYMBOL_REFs. We must emit a blockage insn before restoring $gp > @@ -10900,7 +11299,8 @@ mips_expand_epilogue (bool sibcall_p) >=20 > /* If we don't use shoadow register set, we need to update SP. */ > if (!cfun->machine->use_shadow_register_set_p) > - mips_deallocate_stack (stack_pointer_rtx, GEN_INT (step2), 0); > + mips_deallocate_stack (stack_pointer_rtx, GEN_INT (step2), > + 0, use_jraddiusp_p); > else > /* The choice of position is somewhat arbitrary in this case. */ > mips_epilogue_emit_cfa_restores (); > @@ -10911,9 +11311,11 @@ mips_expand_epilogue (bool sibcall_p) > } > else > /* Deallocate the final bit of the frame. */ > - mips_deallocate_stack (stack_pointer_rtx, GEN_INT (step2), 0); > + mips_deallocate_stack (stack_pointer_rtx, GEN_INT (step2), > + 0, use_jraddiusp_p); > } > - gcc_assert (!mips_epilogue.cfa_restores); > + if (!use_jraddiusp_p) > + gcc_assert (!mips_epilogue.cfa_restores); >=20 > /* Add in the __builtin_eh_return stack adjustment. We need to > use a temporary in MIPS16 code. */ > @@ -10963,6 +11365,10 @@ mips_expand_epilogue (bool sibcall_p) > rtx reg =3D gen_rtx_REG (Pmode, GP_REG_FIRST + 7); > pat =3D gen_return_internal (reg); > } > + else if (use_jraddiusp_p) > + { > + pat =3D gen_mips_jraddiusp (GEN_INT (step2)); > + } > else > { > rtx reg =3D gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM); > ------------------------------------------------------------------------- >=20 > ...unless I'm missing something, it never gets set anyway. >=20 > ------------------------------------------------------------------------- > static void > mips_set_current_function (tree fndecl) > { > - mips_set_mips16_mode (mips_use_mips16_mode_p (fndecl)); > + mips_set_mips16_micromips_mode (mips_use_mips16_mode_p (fndecl), > + mips_use_micromips_mode_p (fndecl)); > + > + /* Override the default setting for function alignment once it is deci= ded > + which mode is in force. */ > + > + if (fndecl > + && TARGET_MICROMIPS > + && optimize_size > + && ! TARGET_INTERLINK_MIPS16 > + && lookup_attribute ("aligned", DECL_ATTRIBUTES (fndecl)) =3D=3D N= ULL) > + DECL_ALIGN (fndecl) =3D 16; > } > ------------------------------------------------------------------------- >=20 > Looks like the wrong place to do this. Please treat this as a separate > patch and get a tree expert to comment. >=20 > ------------------------------------------------------------------------- > +/* Return true if PATTERN matches the kind of instruction generated by > + micromips_build_save_restore. SAVE_P is true for store. */ > + > +bool > +umips_save_restore_pattern_p (bool save_p, rtx pattern) > +{ > + int n; > + HOST_WIDE_INT first_offset =3D 0; > + rtx first_base =3D 0; > + unsigned int first_regno =3D 0; > + > + for (n =3D 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 =3D XVECEXP (pattern, 0, n); > + if (GET_CODE (set) !=3D SET) > + return false; > + > + /* Check that the SET is a load (if restoring) or a store > + (if saving). */ > + mem =3D save_p ? SET_DEST (set) : SET_SRC (set); > + if (!MEM_P (mem)) > + return false; > + > + /* Check that the address is the sum of base and a > + possibly-zero constant offset. */ > + mips_split_plus (XEXP (mem, 0), &this_base, &this_offset); > + if (!REG_P (this_base)) > + return false; > + > + if (n =3D=3D 0) > + { > + first_base =3D this_base; > + first_offset =3D this_offset; > ------------------------------------------------------------------------- >=20 > Need to check the offset is in range. >=20 > ------------------------------------------------------------------------- > + /* Make sure the order of regno is "$16-$23, $30, $31", "$16-$23, = $30", > + or "$31". */ > + this_regno =3D REGNO (reg); > + if (n =3D=3D 0) > + { > + if (this_regno !=3D 16 && this_regno !=3D 31) > + return false; > + first_regno =3D this_regno; > + } > + else if (n =3D=3D 8) /* For s8. */ > + { > + if (n =3D=3D XVECLEN (pattern, 0) - 1) > + { > + if (this_regno !=3D 30 && this_regno !=3D 31) > + return false; > + } > + else > + { > + if (this_regno !=3D 30) > + return false; > + } > + } > + else if (n !=3D XVECLEN (pattern, 0) - 1) > + { > + if (this_regno !=3D first_regno + n) > + return false; > + } > + else /* The last item. */ > + { > + if ((this_regno !=3D first_regno + n) && this_regno !=3D 31) > + return false; > + } > ------------------------------------------------------------------------- >=20 > Please just build a mask of the registers and check it against the > same array as the build function. >=20 > ------------------------------------------------------------------------- > + /* If any item in the list is volatile then disallow. This is a > + safeguard only as this is a path unlikely to be exercised since > + typical code only generates the instructions for stack accesses= . */ > + if (MEM_VOLATILE_P (mem)) > + return false; > ------------------------------------------------------------------------- >=20 > Please add this to the earlier !MEM_P check, i.e.: >=20 > ------------------------------------------------------------------------- > if (!MEM_P (mem) || MEM_VOLATILE_P (mem)) > return false; > ------------------------------------------------------------------------- >=20 > ------------------------------------------------------------------------- > +/* Return the assembly instruction for microMIPS lwm or swm. > + SAVE_P and PATTERN are as for umips_save_restore_pattern_p. */ > + > +const char * > +umips_output_save_restore (bool save_p, rtx pattern) > +{ > + static char buffer[300]; > + char *s; > + int n; > + HOST_WIDE_INT offset; > + rtx base, mem, set, reg, last_set, last_reg; > + > + /* Parse the pattern. */ > + gcc_assert (umips_save_restore_pattern_p (save_p, pattern)); > ------------------------------------------------------------------------- >=20 > Would be nice to have a shared subroutine of this and > umips_save_restore_pattern_p that returns the type/encoding index > (or -1 if none). That'd make this function easier to write. >=20 > ------------------------------------------------------------------------- > + mem1_temp =3D XEXP (mem1, 0); > + mem2_temp =3D XEXP (mem2, 0); > + > + /* Make sure memory is base plus offset. */ > + if (GET_CODE (mem1_temp) !=3D PLUS > + || GET_CODE (mem2_temp) !=3D PLUS > + || GET_CODE (XEXP (mem1_temp, 1)) !=3D CONST_INT > + || GET_CODE (XEXP (mem2_temp, 1)) !=3D CONST_INT) > + return false; > + > + mips_split_plus (mem1_temp, &base1, &offset1); > + mips_split_plus (mem2_temp, &base2, &offset2); > + > + if (!REG_P (base1) || !REG_P (base2)) > + return false; > + > + if (REGNO (base1) !=3D REGNO (base2)) > + return false; > ------------------------------------------------------------------------- >=20 > Better as: >=20 > ------------------------------------------------------------------------- > 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; > ------------------------------------------------------------------------- >=20 > Note that this allows cases where mem1 or mem2 are unoffsetted registers, > which should always be (reg) rather than (plus (reg) (const_int 0)). >=20 > ------------------------------------------------------------------------- > +/* Return the assembly instruction for microMIPS lwp or swp. > + LOAD_P is true for load. */ > + > +const char * > +umips_output_load_store_pair (bool load_p, rtx reg, rtx mem) > +{ > + static char buffer[300]; > + HOST_WIDE_INT offset; > + rtx base; > + > + gcc_assert (REG_P (reg) && MEM_P (mem)); > + > + mips_split_plus (XEXP (mem, 0), &base, &offset); > + gcc_assert (REG_P (base)); > + > + sprintf (buffer, "%s\t%s,%d(%s)", load_p ? "lwp" : "swp", > + reg_names [REGNO (reg)], (int) offset, reg_names [REGNO > (base)]); > + return buffer; > +} > ------------------------------------------------------------------------- >=20 > Instead use output_asm_insn and return "". This allows you to use > the normal assembly formatting. >=20 > ------------------------------------------------------------------------- > + int match[8] =3D {0x00000060, /* 5, 6 */ > + 0x000000a0, /* 5, 7 */ > + 0x000000c0, /* 6, 7 */ > + 0x00200010, /* 4, 21 */ > + 0x00400010, /* 4, 22 */ > + 0x00000030, /* 4, 5 */ > + 0x00000050, /* 4, 6 */ > + 0x00000090}; /* 4, 7 */ > ------------------------------------------------------------------------- >=20 > static const. >=20 > ------------------------------------------------------------------------- > + for (i =3D 0; i < 8; i++) > + { > + if (pair =3D=3D match[i]) > + return true; > + } > ------------------------------------------------------------------------- >=20 > ARRAY_SIZE (match) instead of 8. >=20 > ------------------------------------------------------------------------- > +/* True if this constant is valid for the microMIPS andi16 insn. */ > +#define UMIPS_ANDI16_IMM(VALUE) \ > + (((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 1) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 2) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 3) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 4) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 7) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 8) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 15) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 16) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 31) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 32) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 63) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 64) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 255) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 32768) \ > + || ((unsigned HOST_WIDE_INT) (VALUE) =3D=3D 65536)) > ------------------------------------------------------------------------- >=20 > These casts look redundant. >=20 > Richard