FW: [PATCH] [MIPS] microMIPS gcc support
Richard Sandiford
rdsandiford@googlemail.com
Fri Jul 20 00:46:00 GMT 2012
"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> Forgot to copy the list ...
Same with my reply...
> 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.
This is looking better. At least that huge if statement in
mips_adjust_insn_length (IIRC) is gone :-)
-------------------------------------------------------------------------
@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.
-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-mips16}
therefore disables direct jumps unless GCC knows that the target of the
-jump is not MIPS16.
+jump is not MIPS16/non microMIPS.
-------------------------------------------------------------------------
This doesn't read very well. Let's add a -minterlink-compressed
option and treat -mno-interlink-mips16 as an alias of it. So:
-------------------------------------------------------------------------
@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.
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.
@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.
-------------------------------------------------------------------------
Let's also make TARGET_INTERLINK_COMPRESSED the internal name.
-------------------------------------------------------------------------
+@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@.
-------------------------------------------------------------------------
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.
-------------------------------------------------------------------------
+@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.
-------------------------------------------------------------------------
typo: function
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 instead.
The above is too low-level.
With analogy to -minterlink-compressed, the option could be
-minterlink-uncompressed/-mno-interlink-uncompressed, with the
default being -minterlink-uncompressed.
-------------------------------------------------------------------------
+@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.
-------------------------------------------------------------------------
"For MIPS" is a bit vague (it actually applies to MIPS16 too).
So how about:
-------------------------------------------------------------------------
@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}.
@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}.
-------------------------------------------------------------------------
No need for "YE": we have "ZR" now. I'd like to make the other constraints
"Z" rather than "Y" too -- ZC and ZD -- since "Y" is really for constants.
-------------------------------------------------------------------------
+(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")])
-------------------------------------------------------------------------
Nitlet: local style is to use "{ return ...; }" for single-line statements
and column-0 placement of "{" and "}" for multi-line ones. Same for
rest of file.
-------------------------------------------------------------------------
+; 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 == REGNO (operands[2])
+ && umips_load_store_pair_p (true, false, operands[0],
+ operands[1], operands[3]))
+ || (REGNO (operands[2]) + 1 == 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))])]
-------------------------------------------------------------------------
Please split out this:
&& ((REGNO (operands[0]) + 1 == REGNO (operands[2])
&& umips_load_store_pair_p (true, false, operands[0],
operands[1], operands[3]))
|| (REGNO (operands[2]) + 1 == REGNO (operands[0])
&& umips_load_store_pair_p (true, true, operands[2],
operands[3], operands[1])))
into a function:
bool
umips_load_store_pair_p (bool load_p, rtx reg1, rtx reg2, rtx mem1, rtx mem2)
with the current umips_load_store_pair_p being a subroutine
(umips_load_store_pair_p_1).
-------------------------------------------------------------------------
+(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 == REGNO (operands[2])
+ && umips_load_store_pair_p (true, false, operands[0],
+ operands[1], operands[3]))
+ || (REGNO (operands[2]) + 1 == REGNO (operands[0])
+ && umips_load_store_pair_p (true, true, operands[2],
+ operands[3], operands[1])))"
+ {
+ if (REGNO (operands[0]) + 1 == 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")])
-------------------------------------------------------------------------
Same with the output:
void
umips_output_load_store_pair (bool load_p, rtx reg1, rtx reg2,
rtx mem1, rtx mem2)
Why do only the stores require non_volatile_mem_operand? It looks like
you could be reordering loads from volatile memory otherwise.
-------------------------------------------------------------------------
+; 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")])
-------------------------------------------------------------------------
Redundant comment.
-------------------------------------------------------------------------
+; 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<MOVEP1:mode><MOVEP2:mode>"
+ [(parallel [(set (match_operand:MOVEP1 0 "register_operand")
+ (match_operand:MOVEP1 1 "movep_<MOVEP1:mode>_operand"))
+ (set (match_operand:MOVEP2 2 "register_operand")
+ (match_operand:MOVEP2 3 "movep_<MOVEP2:mode>_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" "<MODE>")
+ (set_attr "can_delay" "no")])
-------------------------------------------------------------------------
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?
-------------------------------------------------------------------------
(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) == 1")))
+ (match_test "mips_address_insns (XEXP (op, 0), mode, false)")))
-------------------------------------------------------------------------
No, this defeats the purpose of "R", which is supposed to disallow
address that require assembly macros.
-------------------------------------------------------------------------
+(define_memory_constraint "YC"
+ "For MIPS, it is the same as the constraint R. For microMIPS, it matches
+ 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 matches
+ 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)")))
-------------------------------------------------------------------------
Is YD internal or not? We should remove it from the docs if so,
otherwise the @internal is wrong.
Please keep the doc strings exactly the same as the .texi version
(including @ 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.
-------------------------------------------------------------------------
+(define_predicate "gpr_operand"
+ (and (match_code "reg")
+ (match_test "GP_REG_P (REGNO (op))")))
-------------------------------------------------------------------------
This is d_operand.
-------------------------------------------------------------------------
+(define_predicate "umips_register"
+ (and (match_code "reg")
+ (match_test "M16_REG_P (REGNO (op))")))
-------------------------------------------------------------------------
I think I'd prefer to call the predicate m16_register too, for consistency.
-------------------------------------------------------------------------
+(define_predicate "umips_andi16_operand"
+ (and (match_code "const")
+ (match_test "UMIPS_ANDI16_IMM (INTVAL (op))")))
-------------------------------------------------------------------------
Looks like this should be "const_int" instead. Same for the other
constants. It seems on face value like some parts of the patch
weren't exercised because of this.
-------------------------------------------------------------------------
+(define_predicate "umips_add_uimm5"
+ (and (match_code "const")
+ (match_test "IN_RANGE (INTVAL (op), -8, 7)")))
-------------------------------------------------------------------------
Looks like a signed rather than unsigned immediate. Should this be
"umips_add_imm5" instead?
-------------------------------------------------------------------------
+(define_predicate "umips_addiur2_imm3"
+ (and (match_code "const")
+ (ior (match_test "INTVAL(op) == 1")
+ (ior (match_test "INTVAL(op) == 4")
+ (ior (match_test "INTVAL(op) == 8")
+ (ior (match_test "INTVAL(op) == 12")
+ (ior (match_test "INTVAL(op) == 16")
+ (ior (match_test "INTVAL(op) == 20")
+ (ior (match_test "INTVAL(op) == 24")
+ (match_test "INTVAL(op) == -1"))))))))))
-------------------------------------------------------------------------
(ior ...) can take any number of operands these days, so no need for
all those ")"s. Space after INTVAL.
-------------------------------------------------------------------------
+(define_predicate "umips_lw16_memref"
+ (and (match_code "plus")
+ (match_operand 0 "umips_register" ""))
+{
+ rtx op2 = XEXP (op, 1);
+
+ return (GET_CODE (op2) == CONST_INT
+ && ((INTVAL (op2) & 3) == 0)
+ && (IN_RANGE (INTVAL (op2), 0, 60)));
+})
-------------------------------------------------------------------------
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:
(and (match_code "plus")
(match_operand 0 "umips_register" ""))
tests for something that is both a PLUS and a REG, which is never true.
((match_operand X ...) doesn't test XEXP (..., X).)
-------------------------------------------------------------------------
+(define_predicate "const_0_si_operand"
+ (and (match_code "const_int,const_double,const_vector")
+ (match_test "op == CONST0_RTX (SImode)")))
+
+(define_predicate "const_0_sf_operand"
+ (and (match_code "const_int,const_double,const_vector")
+ (match_test "op == CONST0_RTX (SFmode)")))
-------------------------------------------------------------------------
Should be redundant with const_0_operand. Modes should be checked
at the match_operand level where important.
-------------------------------------------------------------------------
+(define_predicate "movep_si_operand"
+ (ior (and (match_code "const_int,const_double,const_vector")
+ (match_test "op == 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 == CONST0_RTX (SFmode)"))
+ (and (match_code "reg")
+ (ior (match_test ("IN_RANGE (REGNO (op), 0, 3)"))
+ (match_test ("IN_RANGE (REGNO (op), 16, 20)"))))))
-------------------------------------------------------------------------
These two should be combined:
-------------------------------------------------------------------------
(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)")))))
(define_predicate "movep_operand"
(ior (match_operand 0 "const_0_operand")
(match_operand 0 "movep_register")))
-------------------------------------------------------------------------
-------------------------------------------------------------------------
+(define_predicate "umips_lwp_register"
+ (and (match_code "reg")
+ (match_test "REGNO (op) <= GP_REG_LAST")))
-------------------------------------------------------------------------
Reundant with d_operand/gpr_operand?
-------------------------------------------------------------------------
+;; 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))"))))))
-------------------------------------------------------------------------
This looks wrong: the whole thing is wrapped in (not ...), so it matches
something that specifically _isn't_ a memory_operand. All that reload
stuff shouldn't be needed, plain:
(define_predicate "non_volatile_mem_operand"
(and (match_operand 0 "memory_operand")
(not (match_test "MEM_VOLATILE_P (op)"))))
should be OK.
-------------------------------------------------------------------------
+(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")))
-------------------------------------------------------------------------
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.
-------------------------------------------------------------------------
+(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")))
-------------------------------------------------------------------------
Well, I suppose this OK for now, but it seems odd to be hiding
the difference from the main patterns. Don't we want IRA+reload
to optimise for this where possible? Or does that produce bad results?
More below.
Rather than define lots of attributes, please just use [(cond ...)]
to set the default directly:
(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")
...]
(const_string "no")))
-------------------------------------------------------------------------
+(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")))
-------------------------------------------------------------------------
Patch collision: as of a couple of days ago, we now have an "mfhi"
"type" attribute.
-------------------------------------------------------------------------
+(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")))
-------------------------------------------------------------------------
(eq_attr "alu_type" "and,or,xor").
-------------------------------------------------------------------------
;; 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.
-------------------------------------------------------------------------
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.
-------------------------------------------------------------------------
-------------------------------------------------------------------------
;; 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.
+
-------------------------------------------------------------------------
Excess whitespace.
-------------------------------------------------------------------------
+ (eq_attr "type" "multimem")
+ (const_int 4)
-------------------------------------------------------------------------
Not necessary, 4 is the default.
-------------------------------------------------------------------------
+(define_insn "*branch_equality<mode>_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) <= 8
+ && GET_CODE (operands[3]) == CONST_INT
+ && INTVAL (operands[3]) == 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"));
+}
-------------------------------------------------------------------------
This:
&& GET_CODE (operands[3]) == CONST_INT
&& INTVAL (operands[3]) == 0)
can just be:
&& operands[3] == const0_rtx)
Why the !TARGET_BRANCHLIKELY test? You force it off anyway in
mips_set_mips16_micromips_mode (which I agree is a good thing)
-------------------------------------------------------------------------
(define_insn "indirect_jump_<mode>"
[(set (pc) (match_operand:P 0 "register_operand" "d"))]
""
- "%*j\t%0%/"
+{
+ if (TARGET_MICROMIPS)
+ return "%*jr%:\t%0";
+ else
+ return "%*j\t%0%/";
+}
-------------------------------------------------------------------------
"jr" should be fine across the board. Same later.
-------------------------------------------------------------------------
+ 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.
-------------------------------------------------------------------------
I'd prefer:
-------------------------------------------------------------------------
microMIPS LWM and SWM support 12-bit offsets (from -0x800 to 0x7ff),
so we use a maximum of 0x7f0 for TARGET_MICROMIPS.
-------------------------------------------------------------------------
(where the context already describes the alignment constraint).
-------------------------------------------------------------------------
+static bool
+mips_nocompression_decl_p (const_tree decl)
+{
+ if (lookup_attribute ("nocompression", DECL_ATTRIBUTES (decl)) != NULL)
+ return true;
+
+ return (lookup_attribute ("nomips16", DECL_ATTRIBUTES (decl)) != NULL
+ && lookup_attribute ("nomicromips", DECL_ATTRIBUTES (decl)) != NULL);
+}
...
+/* Similar predicates for "micromips"/"nomicromips" function attributes. */
+
+static bool
+mips_micromips_decl_p (const_tree decl)
+{
+ return lookup_attribute ("micromips", DECL_ATTRIBUTES (decl)) != NULL;
+}
+
+static bool
+mips_nomicromips_decl_p (const_tree decl)
+{
+ return lookup_attribute ("nomicromips", DECL_ATTRIBUTES (decl)) != NULL;
+}
+
+/* Return true if function DECL is a microMIPS function. Return the ambient
+ 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 = decl_function_context (decl);
+ if (parent)
+ decl = parent;
+ if (mips_micromips_decl_p (decl))
+ return true;
+ if (mips_nomicromips_decl_p (decl))
+ return false;
+ }
+ return mips_base_micromips;
+}
-------------------------------------------------------------------------
To reduce the cut-&-paste here and elsewhere, I think it would be
better to have:
-------------------------------------------------------------------------
unsigned int mips_base_compression_flags;
-------------------------------------------------------------------------
to replace mips_base_mips16 & mips_base_micromips and:
-------------------------------------------------------------------------
/* 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;
}
/* Return the compression mode that should be used for function DECL.
Return the ambient setting if DECL is null. */
static unsigned int
mips_get_compress_mode (tree decl)
{
unsigned int flags, force_on;
flags = 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 = decl_function_context (decl);
if (parent)
decl = parent;
force_on = mips_get_compression_on_flags (decl);
if (force_on)
return force_on;
flags &= ~mips_get_compression_off_flags (decl);
}
return flags;
}
/* Return the name of the compression mode represented by target
flags FLAGS. */
static const char *
mips_get_compression_name (unsigned int flags)
{
if (flags & TARGET_MIPS16)
return "mips16";
if (flags & TARGET_MICROMIPS)
return "micromips";
gcc_unreachable();
}
-------------------------------------------------------------------------
then use these instead of tests for individual mips16 or micromips
attributes.
-------------------------------------------------------------------------
switch (addr.type)
{
case ADDRESS_REG:
+
if (TARGET_MIPS16
&& !mips16_unextended_reference_p (mode, addr.reg,
UINTVAL (addr.offset)))
-------------------------------------------------------------------------
Excess whitespace.
-------------------------------------------------------------------------
+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 != BLKmode && might_split_p)
+ factor = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
+ else
+ factor = 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;
+}
-------------------------------------------------------------------------
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:
-------------------------------------------------------------------------
/* Return true if X is a legimate 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)
&& IN_RANGE (INTVAL (addr.offset), -2048, 2047));
}
-------------------------------------------------------------------------
Seems like you could reuse this elsewhere in places that also
check for 12-bit offsets.
-------------------------------------------------------------------------
+/* Return true if the insn has three micromips register operands. */
+int
+umips_three_reg (rtx insn)
+{
+ rtx op0 = XEXP (insn, 0);
+ rtx op1 = XEXP (insn, 1);
+ rtx op2 = XEXP (insn, 2);
+
+ return (op0 != NULL_RTX
+ && op1 != NULL_RTX
+ && op2 != 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)));
+}
-------------------------------------------------------------------------
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.
I think --enable-checking=yes,rtl would have tripped over the op2
extraction and flagged it as a problem. Could you use that option
when testing the updated patch?
Same problem with the other predicates.
Please make the predicates return bool rather than int.
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.
-------------------------------------------------------------------------
+ if (TARGET_MICROMIPS)
+ fprintf (asm_out_file, "\t.set\tmicromips\n");
+ else
+ fprintf (asm_out_file, "\t.set\tnomicromips\n");
-------------------------------------------------------------------------
We need a configure test to see whether the assembler supports microMIPS.
We shouldn't emit ".set\tnomicromips" if it doesn't.
-------------------------------------------------------------------------
+ /* 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 == 0 || final_sequence == 0)
+ putc ('c', file);
-------------------------------------------------------------------------
Should just be "final_sequence == 0". Same for '%!'.
-------------------------------------------------------------------------
+static void mips_save_reg (rtx reg, rtx mem);
-------------------------------------------------------------------------
Please move the functions around so that this isn't necessary.
-------------------------------------------------------------------------
+/* 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. */
-------------------------------------------------------------------------
/* 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.
Return true on success, in which case all GPRs will have been saved. */
-------------------------------------------------------------------------
+static bool
+umips_build_save_restore (mips_save_restore_fn fn,
+ HOST_WIDE_INT offset)
-------------------------------------------------------------------------
Odd formatting.
-------------------------------------------------------------------------
+ unsigned int type[19] = {0x00010000, 0x00030000, 0x00070000, 0x000f0000,
+ 0x001f0000, 0x003f0000, 0x007f0000, 0x00ff0000,
+ 0x40ff0000, 0x80000000, 0x80010000, 0x80030000,
+ 0x80070000, 0x800f0000, 0x801f0000, 0x803f0000,
+ 0x807f0000, 0x80ff0000, 0xc0ff0000};
+ unsigned int encode[19] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 16, 17, 18, 19,
+ 20, 21, 22, 23, 24, 25};
-------------------------------------------------------------------------
Make these static const arrays.
-------------------------------------------------------------------------
+ /* Try matching $16 to $31 (s0 to ra). */
+ for (i = 0; i < 19; i++)
+ if ((cfun->machine->frame.mask & 0xffff0000) == type[i])
+ break;
-------------------------------------------------------------------------
ARRAY_SIZE (type) instead of 19 (here and elsewhere).
-------------------------------------------------------------------------
+ /* For only one register, we use normal sw/lw for speed. */
+ if (i == 0 || i == 9)
+ return false;
-------------------------------------------------------------------------
Seems easier to drop these two elements from the array (with a comment).
-------------------------------------------------------------------------
+ /* For $31 to $24. */
+ if (i < 8 && (cfun->machine->frame.mask & 0xff000000))
+ {
+ int regno;
+ for (regno = 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 -= UNITS_PER_WORD;
+ }
+ }
-------------------------------------------------------------------------
Rather than this, please just use:
-------------------------------------------------------------------------
extra_regs = (cfun->machine->frame.mask & ~type[i]);
for (regno = 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 -= UNITS_PER_WORD;
}
-------------------------------------------------------------------------
-------------------------------------------------------------------------
+ mem = gen_frame_mem (SImode, plus_constant (Pmode, this_base,
+ this_offset + j * UNITS_PER_WORD));
-------------------------------------------------------------------------
Nonstandard formatting. Might be easiest to split out the offset calculation.
-------------------------------------------------------------------------
@@ -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 = 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 = GP_REG_LAST; regno >= 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 -= UNITS_PER_WORD;
}
+save_restore_fp_reg:
+
/* This loop must iterate over the same space as its companion in
mips_compute_frame_info. */
offset = cfun->machine->frame.fp_sp_offset - sp_offset;
-------------------------------------------------------------------------
No!!! :-) Just wrap the GPR save in:
if (!(TARGET_MICROMIPS && umips_build_save_restore (fn, offset)))
Please also split out the save loop, since we now have three copies of it.
-------------------------------------------------------------------------
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 == stack_pointer_rtx && offset == const0_rtx)
return;
mips_frame_barrier ();
+
-------------------------------------------------------------------------
Seems odd to add a parameter that causes the function to return instantly.
But...
-------------------------------------------------------------------------
const struct mips_frame_info *frame;
HOST_WIDE_INT step1, step2;
rtx base, adjust, insn;
+ bool use_jraddiusp_p = false;
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 = MIPS_EPILOGUE_TEMP (Pmode);
}
- mips_deallocate_stack (base, adjust, step2);
+ mips_deallocate_stack (base, adjust, step2, use_jraddiusp_p);
/* 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)
/* 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);
/* 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 = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
pat = gen_return_internal (reg);
}
+ else if (use_jraddiusp_p)
+ {
+ pat = gen_mips_jraddiusp (GEN_INT (step2));
+ }
else
{
rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
-------------------------------------------------------------------------
...unless I'm missing something, it never gets set anyway.
-------------------------------------------------------------------------
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 decided
+ which mode is in force. */
+
+ if (fndecl
+ && TARGET_MICROMIPS
+ && optimize_size
+ && ! TARGET_INTERLINK_MIPS16
+ && lookup_attribute ("aligned", DECL_ATTRIBUTES (fndecl)) == NULL)
+ DECL_ALIGN (fndecl) = 16;
}
-------------------------------------------------------------------------
Looks like the wrong place to do this. Please treat this as a separate
patch and get a tree expert to comment.
-------------------------------------------------------------------------
+/* 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 = 0;
+ rtx first_base = 0;
+ unsigned int first_regno = 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))
+ 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 == 0)
+ {
+ first_base = this_base;
+ first_offset = this_offset;
-------------------------------------------------------------------------
Need to check the offset is in range.
-------------------------------------------------------------------------
+ /* Make sure the order of regno is "$16-$23, $30, $31", "$16-$23, $30",
+ or "$31". */
+ this_regno = REGNO (reg);
+ if (n == 0)
+ {
+ if (this_regno != 16 && this_regno != 31)
+ return false;
+ first_regno = this_regno;
+ }
+ else if (n == 8) /* For s8. */
+ {
+ if (n == XVECLEN (pattern, 0) - 1)
+ {
+ if (this_regno != 30 && this_regno != 31)
+ return false;
+ }
+ else
+ {
+ if (this_regno != 30)
+ return false;
+ }
+ }
+ else if (n != XVECLEN (pattern, 0) - 1)
+ {
+ if (this_regno != first_regno + n)
+ return false;
+ }
+ else /* The last item. */
+ {
+ if ((this_regno != first_regno + n) && this_regno != 31)
+ return false;
+ }
-------------------------------------------------------------------------
Please just build a mask of the registers and check it against the
same array as the build function.
-------------------------------------------------------------------------
+ /* 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;
-------------------------------------------------------------------------
Please add this to the earlier !MEM_P check, i.e.:
-------------------------------------------------------------------------
if (!MEM_P (mem) || MEM_VOLATILE_P (mem))
return false;
-------------------------------------------------------------------------
-------------------------------------------------------------------------
+/* 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));
-------------------------------------------------------------------------
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.
-------------------------------------------------------------------------
+ mem1_temp = XEXP (mem1, 0);
+ mem2_temp = XEXP (mem2, 0);
+
+ /* Make sure memory is base plus offset. */
+ if (GET_CODE (mem1_temp) != PLUS
+ || GET_CODE (mem2_temp) != PLUS
+ || GET_CODE (XEXP (mem1_temp, 1)) != CONST_INT
+ || GET_CODE (XEXP (mem2_temp, 1)) != 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) != REGNO (base2))
+ return false;
-------------------------------------------------------------------------
Better as:
-------------------------------------------------------------------------
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;
-------------------------------------------------------------------------
Note that this allows cases where mem1 or mem2 are unoffsetted registers,
which should always be (reg) rather than (plus (reg) (const_int 0)).
-------------------------------------------------------------------------
+/* 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;
+}
-------------------------------------------------------------------------
Instead use output_asm_insn and return "". This allows you to use
the normal assembly formatting.
-------------------------------------------------------------------------
+ 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 */
-------------------------------------------------------------------------
static const.
-------------------------------------------------------------------------
+ for (i = 0; i < 8; i++)
+ {
+ if (pair == match[i])
+ return true;
+ }
-------------------------------------------------------------------------
ARRAY_SIZE (match) instead of 8.
-------------------------------------------------------------------------
+/* True if this constant is valid for the microMIPS andi16 insn. */
+#define UMIPS_ANDI16_IMM(VALUE) \
+ (((unsigned HOST_WIDE_INT) (VALUE) == 1) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 2) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 3) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 4) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 7) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 8) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 15) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 16) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 31) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 32) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 63) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 64) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 255) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 32768) \
+ || ((unsigned HOST_WIDE_INT) (VALUE) == 65536))
-------------------------------------------------------------------------
These casts look redundant.
Richard
More information about the Gcc-patches
mailing list