Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes: 2013-03-21 Catherine Moore c...@codesourcery.com * config/mips/constraints.md (u, Udb7 Uead, Uean, Uesp, Uib3, Uuw6, Usb4, ZS, ZT, ZU, ZV, ZW): New constraints. * config/mip/predicates.md (lwsp_swsp_operand, lw16_sw16_operand, lhu16_sh16_operand, lbu16_operand, sb16_operand, db4_operand, db7_operand, ib3_operand, sb4_operand, ub4_operand, uh4_operand, uw4_operand, uw5_operand, uw6_operand, addiur2_operand, addiusp_operand, andi16_operand): New predicates. * config/mips/mips.md (compression): New attribute. (enabled): New attribute. (length): Consider compression in computing length. (shift_compression): New code attribute. (*addmode3): New operands. Record compression. (submode3): Likewise. (one_cmplmode2): Likewise. (*andmode3): Likewise. (*iormode3): Likewise. (unnamed pattern for xor): Likewise. (*zero_extendSHORT:modeGPR:mode2): Likewise. (*optabmode3): Likewise. (*movmode_internal: Likewise. * config/mips/mips-protos.h (mips_signed_immediate_p): New. (mips_unsigned_immediate_p): New. (umips_lwsp_swsp_address_p): New. (m16_based_address_p): New. * config/mips/mips-protos.h (mips_signed_immediate_p): New prototype. (mips_unsigned_immediate_p): New prototype. (lwsp_swsp_address_p): New prototype. (m16_based_address_p): New prototype. * config/mips/mips.c (mips_unsigned_immediate_p): New function. (mips_signed_immediate_p): New function. (m16_based_address_p): New function. (lwsp_swsp_address_p): New function. (mips_print_operand_punctuation): Recognize short delay slot insns for microMIPS.addmode3 OK. Thanks for your patience through all this. Now the framework's been sorted out, the review process for future encoding patches should be much less painful. Richard
RE: FW: [PATCH] [MIPS] microMIPS gcc support
-Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Thursday, March 21, 2013 8:04 PM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support Thanks, this is almost there now. It was only the problem with the new version of the move pattern (see below) that stopped this from being OK with The next round should be a formality though. Okay, modifications now made and patch attached. Moore, Catherine catherine_mo...@mentor.com writes: +(define_constraint Uuw6 + @internal + An unsigned constant of 6 bits. + (match_operand 0 uw6_operand)) , shifted left two places. + (and (ior (eq_attr compression micromips) + (eq_attr compression all)) Please use (eq_attr compression micromips,all) instead. (define_insn submode3 - [(set (match_operand:GPR 0 register_operand =d) - (minus:GPR (match_operand:GPR 1 register_operand d) - (match_operand:GPR 2 register_operand d)))] + [(set (match_operand:GPR 0 register_operand =!u,d) + (minus:GPR (match_operand:GPR 1 register_operand !u,d) + (match_operand:GPR 2 register_operand !u,d)))] - dsubu\t%0,%1,%2 + @ + dsubu\t%0,%1,%2 + dsubu\t%0,%1,%2 This change isn't needed. It's OK (and IMO better) to keep a single asm string when the string is the same for all alternatives. @@ -4362,13 +4400,14 @@ ;; in FP registers (off by default, use -mdebugh to enable). (define_insn *movmode_internal - [(set (match_operand:IMOVE32 0 nonimmediate_operand =d,d,e,d,m,*f,*f,*d,*m,*d,*z,*a,*d,*B*C*D,*B*C*D,*d,*m) - (match_operand:IMOVE32 1 move_operand d,Yd,Yf,m,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D))] + [(set (match_operand:IMOVE32 0 nonimmediate_operand =d,!u,d,e,!u,!u,!u,!u,!ks,d,ZS,ZV,ZU,ZT,m,*f,*f,*d,*m,*d,*z,*a,*d,*B*C*D, *B*C*D,*d,*m) + (match_operand:IMOVE32 1 move_operand +d,J,Yd,Yf,Udb7,ZW,ZU,ZT,ZS,m,!ks,!u,!u,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J* +d,*a,*d,*m,*B*C*D,*B*C*D))] This pattern is only for 32-bit moves, so should only use ZS and ZT. (I don't mind keeping the definitions of the ZU...ZW constraints in this patch - - even though they start out unused -- because it's obvious they'll be needed eventually. I'd prefer leaving the 8-bit and 16-move patterns themselves to a different patch though.) Sorry, I should have noticed last time, but alternative 5 (u-Udb7) should come before alternative 3 (d-Yd), because d-Yd includes everything that u-Udb7 does. +extern bool m16_based_address_p (rtx, enum machine_mode, int +(*)(rtx_def*, machine_mode)); Line too long: should be split after the first enum machine_mode. Thanks, Richard short-delay-03-22.cl Description: short-delay-03-22.cl short-delay-03-22.patch Description: short-delay-03-22.patch
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
I'm sorry for wasting your time. I accidentally posted an older version
of the patch earlier this afternoon.
This is the version that I meant to post and is hopefully a lot closer
to what you are looking for.
I named some of the predicates/constraints differently than your current
suggestions, but hopefully they will be OK.
Yeah, that's fine.
I'll fold my comments from yesterday into the reply to this patch.
Index: constraints.md
===
--- constraints.md(revision 196638)
+++ constraints.md(working copy)
@@ -43,6 +43,9 @@
(define_register_constraint b ALL_REGS
@internal)
+(define_register_constraint u M16_REGS
+ @internal)
+
;; MIPS16 code always calls through a MIPS16 register; see
mips_emit_call_insn
;; for details.
(define_register_constraint c TARGET_MIPS16 ? M16_REGS
@@ -170,6 +173,41 @@
(and (match_operand 0 call_insn_operand)
(match_test CONSTANT_P (op
+(define_constraint Udb7
+ @internal
+ A decremented unsigned constant of 7 bits.
+ (match_operand 0 db7_operand))
+
+(define_constraint Uead
+ @internal
+ A microMIPS encoded ADDIUR2 immediate operand.
+ (match_operand 0 addiur2_operand))
+
+(define_constraint Uean
+ @internal
+ A microMIPS encoded andi operand.
+ (match_operand 0 andi16_operand))
s/andi/ANDI/
+(define_constraint Uesp
+ @internal
+ A microMIPS encoded ADDIUR1SP operand.
+ (match_operand 0 addiur1sp_operand))
This is 6 bits zero extended and shifted left twice, so the constraint
should be Uuw6 and the predicate should be uw6_operand. Redundant .
That frees up Uesp for something else, so it might be worth using Uesp
(instead of Ueim) for ADDIUSP.
+(define_memory_constraint ZS
+ @internal
+ A microMIPS memory operand for use with the LWSP/SWSP insns.
+ (and (match_code mem)
+ (match_operand 0 lwsp_swsp_operand)))
+
+(define_memory_constraint ZT
+ @internal
+ A microMIPS memory operand for use with the LW16 insn.
+ (and (match_code mem)
+ (match_operand 0 lw16_operand)))
As with LWSP/SWSP, we shouldn't need to split the LW16 and SW16 cases.
One constraint is enough for both. We only need different constraints
if the offsets have different ranges. I think that means one for
LW16/SW16, one for LH16/SH16, one for LB16 and one for SB16.
+(define_predicate sw16_operand
+ (ior (and (match_code mem)
+ (match_test m16_based_address_p (XEXP (op, 0), mode,
uw4_operand)))
+ (and (match_code reg)
+ (match_test REGNO (op) == GP_REG_FIRST
I don't understand the reg case. The operand is used to match the
destination of a store, so it really should be a mem in all cases.
Same for the other stores.
+(define_predicate db4_operand
+ (and (match_code const_int)
+ (match_test mips_unsigned_immediate_p (INTVAL (op) - 1, 4, 0
+1 rather than -1.
+(define_predicate addiur1sp_operand
+ (and (match_code const_int)
+ (match_test mips_unsigned_immediate_p (INTVAL (op), 8, 2
Watch out when renaming this to uw6_operand. It should be 6, 2 rather
than 8, 2.
@@ -1131,14 +1151,19 @@
)
(define_insn *addmode3
- [(set (match_operand:GPR 0 register_operand =d,d)
- (plus:GPR (match_operand:GPR 1 register_operand d,d)
- (match_operand:GPR 2 arith_operand d,Q)))]
+ [(set (match_operand:GPR 0 register_operand =!u,d,!u,!ks,d,d,d)
+ (plus:GPR (match_operand:GPR 1 register_operand !u,d,!u,0,!ks,!ks,d)
+ (match_operand:GPR 2 arith_operand
!u,d,Uead,Usb3,Ueim,Uesp,Q)))]
Hmm, not sure about these alternatives. Two have the form d/ks/U...
(the last two) and I'm not sure which instruction Usb3 corresponds to.
My guess from the manual would be:
[(set (match_operand:GPR 0 register_operand =!u,d,!u,!u,!ks,!d,d)
(plus:GPR (match_operand:GPR 1 register_operand !u,d,!u,!ks,!ks,0,d)
(match_operand:GPR 2 arith_operand
!u,d,Uead,Uuw6,Ueim,Usb4,Q)))]
@@ -1347,12 +1372,13 @@
(set_attr mode UNITMODE)])
(define_insn submode3
- [(set (match_operand:GPR 0 register_operand =d)
- (minus:GPR (match_operand:GPR 1 register_operand d)
-(match_operand:GPR 2 register_operand d)))]
+ [(set (match_operand:GPR 0 register_operand =!u,d)
+ (minus:GPR (match_operand:GPR 1 register_operand !u,d)
+(match_operand:GPR 2 register_operand !u,d)))]
- dsubu\t%0,%1,%2
+{ return dsubu\t%0,%1,%2; }
The change to the last line shouldn't be needed. (Plain ...
is better than { return ; } where possible, because the
generator doesn't need to create a separate output function.)
@@ -4362,13 +4398,14 @@
;; in FP registers (off by default, use -mdebugh to enable).
(define_insn *movmode_internal
- [(set (match_operand:IMOVE32 0 nonimmediate_operand
RE: FW: [PATCH] [MIPS] microMIPS gcc support
-Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Thursday, March 21, 2013 4:05 AM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support Moore, Catherine catherine_mo...@mentor.com writes: I'm sorry for wasting your time. I accidentally posted an older version of the patch earlier this afternoon. This is the version that I meant to post and is hopefully a lot closer to what you are looking for. I named some of the predicates/constraints differently than your current suggestions, but hopefully they will be OK. Yeah, that's fine. I'll fold my comments from yesterday into the reply to this patch. Okay, thanks. Updated patch attached. I'm still testing, but initial results are good. Catherine Index: constraints.md == = --- constraints.md (revision 196638) +++ constraints.md (working copy) @@ -43,6 +43,9 @@ (define_register_constraint b ALL_REGS @internal) +(define_register_constraint u M16_REGS + @internal) + ;; MIPS16 code always calls through a MIPS16 register; see mips_emit_call_insn ;; for details. (define_register_constraint c TARGET_MIPS16 ? M16_REGS @@ -170,6 +173,41 @@ (and (match_operand 0 call_insn_operand) (match_test CONSTANT_P (op +(define_constraint Udb7 + @internal + A decremented unsigned constant of 7 bits. + (match_operand 0 db7_operand)) + +(define_constraint Uead + @internal + A microMIPS encoded ADDIUR2 immediate operand. + (match_operand 0 addiur2_operand)) + +(define_constraint Uean + @internal + A microMIPS encoded andi operand. + (match_operand 0 andi16_operand)) s/andi/ANDI/ +(define_constraint Uesp + @internal + A microMIPS encoded ADDIUR1SP operand. + (match_operand 0 addiur1sp_operand)) This is 6 bits zero extended and shifted left twice, so the constraint should be Uuw6 and the predicate should be uw6_operand. Redundant . I couldn' t find the redundant . I'm sure you'll let me know if it's still there. That frees up Uesp for something else, so it might be worth using Uesp (instead of Ueim) for ADDIUSP. +(define_memory_constraint ZS + @internal + A microMIPS memory operand for use with the LWSP/SWSP insns. + (and (match_code mem) + (match_operand 0 lwsp_swsp_operand))) + +(define_memory_constraint ZT + @internal + A microMIPS memory operand for use with the LW16 insn. + (and (match_code mem) + (match_operand 0 lw16_operand))) As with LWSP/SWSP, we shouldn't need to split the LW16 and SW16 cases. One constraint is enough for both. We only need different constraints if the offsets have different ranges. I think that means one for LW16/SW16, one for LH16/SH16, one for LB16 and one for SB16. +(define_predicate sw16_operand + (ior (and (match_code mem) + (match_test m16_based_address_p (XEXP (op, 0), mode, uw4_operand))) + (and (match_code reg) + (match_test REGNO (op) == GP_REG_FIRST I don't understand the reg case. The operand is used to match the destination of a store, so it really should be a mem in all cases. Same for the other stores. This was a thinko on my part (and the reason why the LW16/SW16 cases had separate constraints). Now fixed. +(define_predicate db4_operand + (and (match_code const_int) + (match_test mips_unsigned_immediate_p (INTVAL (op) - 1, 4, +0 +1 rather than -1. +(define_predicate addiur1sp_operand + (and (match_code const_int) + (match_test mips_unsigned_immediate_p (INTVAL (op), 8, 2 Watch out when renaming this to uw6_operand. It should be 6, 2 rather than 8, 2. @@ -1131,14 +1151,19 @@ ) (define_insn *addmode3 - [(set (match_operand:GPR 0 register_operand =d,d) - (plus:GPR (match_operand:GPR 1 register_operand d,d) - (match_operand:GPR 2 arith_operand d,Q)))] + [(set (match_operand:GPR 0 register_operand =!u,d,!u,!ks,d,d,d) + (plus:GPR (match_operand:GPR 1 register_operand !u,d,!u,0,!ks,!ks,d) + (match_operand:GPR 2 arith_operand +!u,d,Uead,Usb3,Ueim,Uesp,Q)))] Hmm, not sure about these alternatives. Two have the form d/ks/U... (the last two) and I'm not sure which instruction Usb3 corresponds to. My guess from the manual would be: [(set (match_operand:GPR 0 register_operand =!u,d,!u,!u,!ks,!d,d) (plus:GPR (match_operand:GPR 1 register_operand !u,d,!u,!ks,!ks,0,d) (match_operand:GPR 2 arith_operand !u,d,Uead,Uuw6,Ueim,Usb4,Q)))] Yes, looks right. Usb3 should be Usb4. @@ -1347,12 +1372,13 @@ (set_attr mode UNITMODE)]) (define_insn submode3 - [(set (match_operand:GPR 0 register_operand =d) - (minus:GPR (match_operand:GPR 1
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Thanks, this is almost there now. It was only the problem with the new version of the move pattern (see below) that stopped this from being OK with The next round should be a formality though. Moore, Catherine catherine_mo...@mentor.com writes: +(define_constraint Uuw6 + @internal + An unsigned constant of 6 bits. + (match_operand 0 uw6_operand)) , shifted left two places. + (and (ior (eq_attr compression micromips) + (eq_attr compression all)) Please use (eq_attr compression micromips,all) instead. (define_insn submode3 - [(set (match_operand:GPR 0 register_operand =d) - (minus:GPR (match_operand:GPR 1 register_operand d) -(match_operand:GPR 2 register_operand d)))] + [(set (match_operand:GPR 0 register_operand =!u,d) + (minus:GPR (match_operand:GPR 1 register_operand !u,d) +(match_operand:GPR 2 register_operand !u,d)))] - dsubu\t%0,%1,%2 + @ + dsubu\t%0,%1,%2 + dsubu\t%0,%1,%2 This change isn't needed. It's OK (and IMO better) to keep a single asm string when the string is the same for all alternatives. @@ -4362,13 +4400,14 @@ ;; in FP registers (off by default, use -mdebugh to enable). (define_insn *movmode_internal - [(set (match_operand:IMOVE32 0 nonimmediate_operand =d,d,e,d,m,*f,*f,*d,*m,*d,*z,*a,*d,*B*C*D,*B*C*D,*d,*m) - (match_operand:IMOVE32 1 move_operand d,Yd,Yf,m,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D))] + [(set (match_operand:IMOVE32 0 nonimmediate_operand =d,!u,d,e,!u,!u,!u,!u,!ks,d,ZS,ZV,ZU,ZT,m,*f,*f,*d,*m,*d,*z,*a,*d,*B*C*D,*B*C*D,*d,*m) + (match_operand:IMOVE32 1 move_operand d,J,Yd,Yf,Udb7,ZW,ZU,ZT,ZS,m,!ks,!u,!u,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D))] This pattern is only for 32-bit moves, so should only use ZS and ZT. (I don't mind keeping the definitions of the ZU...ZW constraints in this patch -- even though they start out unused -- because it's obvious they'll be needed eventually. I'd prefer leaving the 8-bit and 16-move patterns themselves to a different patch though.) Sorry, I should have noticed last time, but alternative 5 (u-Udb7) should come before alternative 3 (d-Yd), because d-Yd includes everything that u-Udb7 does. +extern bool m16_based_address_p (rtx, enum machine_mode, int (*)(rtx_def*, machine_mode)); Line too long: should be split after the first enum machine_mode. Thanks, Richard
RE: FW: [PATCH] [MIPS] microMIPS gcc support
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Tuesday, March 19, 2013 4:38 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Tuesday, March 19, 2013 3:26 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Tuesday, March 05, 2013 4:06 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support:
We have a few internal-only undocumented constraints that
aren't used much, so we should be able to move them to the Y
space
instead.
The patch below does this for T and U. Then we could use U
for new, longer constraints.
Utypefactorbits
where type is:
s for signed
u for unsigned
d for decremented unsigned (-1 ... N)
i for incremented unsigned (1 ... N)
where factor is:
b for byte (*1)
h for halfwords (*2)
w for words (*4)
d for doublewords (*8) -- useful for 64-bit MIPS16 but probably
not
needed for 32-bit microMIPS
and where bits is the number of bits. type and factor
could be replaced with an ad-hoc two-letter combination for special
cases.
E.g. Uas9 (add stack) for ADDISUP.
Just a suggestion though. I'm not saying these names are
totally intuitive or anything, but they should at least be
better than arbitrary
letters.
Also, bits could be two digits if necessary, or we could just
use hex
digits.
I extended this proposal a bit by:
1. Adding a type e for encoded. The constraint will start
with Ue, when the operand is an encoded value.
2. I decided to use two digits for bits.
3. The ad-hoc combination is used for anything else.
First of all, thanks for coming up with a counter-suggestion. I'm
hopeless at naming things, so I was hoping there would be at least
some
pushback.
e for encoded sounds good. I'm less keen on the mixture of
single- and double-digit widths though (single digit for some
Ues, double digits for other Us.) I think we should either:
(a) use the same number of digits for all U constraints. That leaves
one character for the Ue type, which isn't as mnemonic, but is in
line with what we do elsewhere.
(b) avoid digits in the Ue forms and just have an ad-hoc letter
combination.
FWIW, as far as this point goes, the patch still has Uea4.
+/* Return true if X is a legitimate address that conforms to
+the
requirements
+ for any of the short microMIPS LOAD or STORE instructions
+ except
LWSP
+ or SWSP. */
+
+bool
+umips_address_p (rtx x, bool load, enum machine_mode mode) {
+
+ struct mips_address_info addr;
+ bool ok = mips_classify_address (addr, x, mode, false)
+ addr.type == ADDRESS_REG
+ M16_REG_P (REGNO (addr.reg))
+ CONST_INT_P (addr.offset);
+
+ if (!ok)
+return false;
+
+ if (mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2)
+ || mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 1))
+return true;
+
+ if (load IN_RANGE (INTVAL (addr.offset), -1, 14))
+return true;
+
+ if (!load IN_RANGE (INTVAL (addr.offset), 0, 15))
+return true;
+
+ return false;
+
+}
No blank lines after { and before }.
More importantly, what cases are these range tests covering?
Both binutils and qemu seem to think that LW and SW need offsets
that exactly match:
mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2)
Those range tests are for the LBU16 and SB16 instructions. LBU16
supports offsets from -1 to 14 (encodes -1 as 15) while the SB16
insn supports offsets from 0-15.
They need to use separate constraints in that case. The patch as
written allows -1 offsets for LW16 too. (In rare cases, offsets like
-1 can be used even with aligned word loads.)
E.g. we could have:
/* Return true if X is legitimate for accessing values of mode MODE,
if it is based on a MIPS16 register, and if the offset satisfies
OFFSET_PREDICATE. */
bool
m16_based_address_p (rtx x, enum machine_mode mode,
insn_operand_predicate_fn predicate) {
struct mips_address_info addr;
return (mips_classify_address (addr, x, mode, false)
addr.type == ADDRESS_REG
M16_REG_P (REGNO (addr.reg
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
Index: gcc/config/mips/constraints.md
===
--- gcc/config/mips/constraints.md(revision 196638)
+++ gcc/config/mips/constraints.md(working copy)
@@ -43,6 +43,9 @@
(define_register_constraint b ALL_REGS
@internal)
+(define_register_constraint u M16_REGS
+ @internal)
+
;; MIPS16 code always calls through a MIPS16 register; see
mips_emit_call_insn
;; for details.
(define_register_constraint c TARGET_MIPS16 ? M16_REGS
@@ -170,6 +173,41 @@
(and (match_operand 0 call_insn_operand)
(match_test CONSTANT_P (op
+(define_constraint Udb7
+ @internal
+ A decremented unsigned constant of 7 bits.
+ (match_operand 0 db7_operand))
+
+(define_constraint Uea4
+ @internal
+ A microMIPS encoded ADDIUR2 immediate operand.
+ (match_operand 0 addiur2_operand))
I'm still wary of having digits for some Ues and not others,
especially because (as I understand it) ADDIUR2 takes a 3-bit rather
than 4-bit operand. How about Ueau here (= add of u register).
For consistency...
+(define_constraint Ueim
+ @internal
+ A microMIPS encoded ADDIUSP operand.
+ (match_operand 0 addiusp_operand ))
...this could be Ueas (= add of stack register). Redundant .
+(define_constraint Uesp
+ @internal
+ A microMIPS encoded ADDIUR1SP operand.
+ (match_operand 0 addiur1sp_operand ))
This is 6 bits zero extended and shifted left twice, so the constraint
should be Uuw6 and the predicate should be uw6_operand. Redundant .
+(define_memory_constraint ZS
+ @internal
+ A microMIPS memory operand for use with the LWSP/SWSP insns.
+ (and (match_code mem)
+ (match_operand 0 umips_lwsp_swsp_operand)))
+
+(define_memory_constraint ZT
+ @internal
+ A microMIPS memory operand for use with the various LOAD insns.
+ (and (match_code mem)
+ (match_operand 0 umips_load_operand)))
+
+(define_memory_constraint ZU
+ @internal
+ A microMIPS memory operand for use with the various STORE insns.
+ (and (match_code mem)
+ (match_operand 0 umips_store_operand)))
As I mentioned before, we need a separate predicate and constraint for
each range. We can't have things like:
+ /* For the LBU16 insn. */
+ if (load db4_operand (addr.offset, mode))
+return true;
+
+ /* For the SB16 insn. */
+ if (!load ub4_operand (addr.offset, mode))
+return true;
+
+ /* For SH16, LHU16, SW16 and LW16 insns. */
+ return uw4_operand (addr.offset, mode)
+ || uh4_operand (addr.offset, mode);
because this allows SH16 to be ub4_operand, uw4_operand or uh4_operand,
and so on. There needs to be one predicate/constraint pair for ub4_operands,
one for db4_operands, one for uw4_operands and one for uh4_operands.
Please use the function I suggested in my previous reply, in which the
offset predicate is passed in as a function pointer.
@@ -1131,14 +1151,19 @@
)
(define_insn *addmode3
- [(set (match_operand:GPR 0 register_operand =d,d)
- (plus:GPR (match_operand:GPR 1 register_operand d,d)
- (match_operand:GPR 2 arith_operand d,Q)))]
+ [(set (match_operand:GPR 0 register_operand =!u,d,!u,!ks,d,d,d)
+ (plus:GPR (match_operand:GPR 1 register_operand !u,d,!u,0,!ks,!ks,d)
+ (match_operand:GPR 2 arith_operand
!u,d,Uea4,Usb3,Ueim,Uesp,Q)))]
Hmm, not sure about these alternatives. Two have the form d/ks/U...
(the last two) and I'm not sure which instruction Usb3 corresponds to.
My guess from the manual would be:
[(set (match_operand:GPR 0 register_operand =!u,d,!u,!u,!ks,!d,d)
(plus:GPR (match_operand:GPR 1 register_operand !u,d,!u,!ks,!ks,0,d)
(match_operand:GPR 2 arith_operand
!u,d,Ueau,Uuw6,Ueas,Usb4,Q)))]
@@ -1347,12 +1372,13 @@
(set_attr mode UNITMODE)])
(define_insn submode3
- [(set (match_operand:GPR 0 register_operand =d)
- (minus:GPR (match_operand:GPR 1 register_operand d)
-(match_operand:GPR 2 register_operand d)))]
+ [(set (match_operand:GPR 0 register_operand =!u,d)
+ (minus:GPR (match_operand:GPR 1 register_operand !u,d)
+(match_operand:GPR 2 register_operand !u,d)))]
- dsubu\t%0,%1,%2
+{ return dsubu\t%0,%1,%2; }
The change to the last line shouldn't be needed. (Plain ...
is better than { return ; } where possible, because the
generator doesn't need to create a separate output function.)
@@ -2878,9 +2905,9 @@
;; register =op1 x
(define_insn *andmode3
- [(set (match_operand:GPR 0 register_operand =d,d,d,d,d,d,d)
- (and:GPR (match_operand:GPR 1 nonimmediate_operand o,o,W,d,d,d,d)
- (match_operand:GPR 2 and_operand Yb,Yh,Yw,K,Yx,Yw,d)))]
+ [(set (match_operand:GPR 0 register_operand =d,d,d,!u,d,d,d,!u,d)
+ (and:GPR (match_operand:GPR 1 nonimmediate_operand
o,o,W,!u,d,d,d,0,d)
RE: FW: [PATCH] [MIPS] microMIPS gcc support
Hi Richard, I'm sorry for wasting your time. I accidentally posted an older version of the patch earlier this afternoon. This is the version that I meant to post and is hopefully a lot closer to what you are looking for. I named some of the predicates/constraints differently than your current suggestions, but hopefully they will be OK. Thanks, Catherine -Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Wednesday, March 20, 2013 5:48 PM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support Moore, Catherine catherine_mo...@mentor.com writes: Index: gcc/config/mips/constraints.md == = --- gcc/config/mips/constraints.md (revision 196638) +++ gcc/config/mips/constraints.md (working copy) @@ -43,6 +43,9 @@ (define_register_constraint b ALL_REGS @internal) +(define_register_constraint u M16_REGS + @internal) + ;; MIPS16 code always calls through a MIPS16 register; see mips_emit_call_insn ;; for details. (define_register_constraint c TARGET_MIPS16 ? M16_REGS @@ -170,6 +173,41 @@ (and (match_operand 0 call_insn_operand) (match_test CONSTANT_P (op +(define_constraint Udb7 + @internal + A decremented unsigned constant of 7 bits. + (match_operand 0 db7_operand)) + +(define_constraint Uea4 + @internal + A microMIPS encoded ADDIUR2 immediate operand. + (match_operand 0 addiur2_operand)) I'm still wary of having digits for some Ues and not others, especially because (as I understand it) ADDIUR2 takes a 3-bit rather than 4-bit operand. How about Ueau here (= add of u register). For consistency... +(define_constraint Ueim + @internal + A microMIPS encoded ADDIUSP operand. + (match_operand 0 addiusp_operand )) ...this could be Ueas (= add of stack register). Redundant . +(define_constraint Uesp + @internal + A microMIPS encoded ADDIUR1SP operand. + (match_operand 0 addiur1sp_operand )) This is 6 bits zero extended and shifted left twice, so the constraint should be Uuw6 and the predicate should be uw6_operand. Redundant . +(define_memory_constraint ZS + @internal + A microMIPS memory operand for use with the LWSP/SWSP insns. + (and (match_code mem) + (match_operand 0 umips_lwsp_swsp_operand))) + +(define_memory_constraint ZT + @internal + A microMIPS memory operand for use with the various LOAD insns. + (and (match_code mem) + (match_operand 0 umips_load_operand))) + +(define_memory_constraint ZU + @internal + A microMIPS memory operand for use with the various STORE insns. + (and (match_code mem) + (match_operand 0 umips_store_operand))) As I mentioned before, we need a separate predicate and constraint for each range. We can't have things like: + /* For the LBU16 insn. */ + if (load db4_operand (addr.offset, mode)) +return true; + + /* For the SB16 insn. */ + if (!load ub4_operand (addr.offset, mode)) +return true; + + /* For SH16, LHU16, SW16 and LW16 insns. */ return uw4_operand + (addr.offset, mode) + || uh4_operand (addr.offset, mode); because this allows SH16 to be ub4_operand, uw4_operand or uh4_operand, and so on. There needs to be one predicate/constraint pair for ub4_operands, one for db4_operands, one for uw4_operands and one for uh4_operands. Please use the function I suggested in my previous reply, in which the offset predicate is passed in as a function pointer. @@ -1131,14 +1151,19 @@ ) (define_insn *addmode3 - [(set (match_operand:GPR 0 register_operand =d,d) - (plus:GPR (match_operand:GPR 1 register_operand d,d) - (match_operand:GPR 2 arith_operand d,Q)))] + [(set (match_operand:GPR 0 register_operand =!u,d,!u,!ks,d,d,d) + (plus:GPR (match_operand:GPR 1 register_operand !u,d,!u,0,!ks,!ks,d) + (match_operand:GPR 2 arith_operand +!u,d,Uea4,Usb3,Ueim,Uesp,Q)))] Hmm, not sure about these alternatives. Two have the form d/ks/U... (the last two) and I'm not sure which instruction Usb3 corresponds to. My guess from the manual would be: [(set (match_operand:GPR 0 register_operand =!u,d,!u,!u,!ks,!d,d) (plus:GPR (match_operand:GPR 1 register_operand !u,d,!u,!ks,!ks,0,d) (match_operand:GPR 2 arith_operand !u,d,Ueau,Uuw6,Ueas,Usb4,Q)))] @@ -1347,12 +1372,13 @@ (set_attr mode UNITMODE)]) (define_insn submode3 - [(set (match_operand:GPR 0 register_operand =d) - (minus:GPR (match_operand:GPR 1 register_operand d) - (match_operand:GPR 2 register_operand d)))] + [(set (match_operand:GPR 0 register_operand =!u,d) + (minus:GPR (match_operand:GPR 1 register_operand !u,d) + (match_operand:GPR 2
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Tuesday, March 05, 2013 4:06 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support:
We have a few internal-only undocumented constraints that aren't used
much, so we should be able to move them to the Y space instead.
The patch below does this for T and U. Then we could use U for
new, longer constraints.
Utypefactorbits
where type is:
s for signed
u for unsigned
d for decremented unsigned (-1 ... N)
i for incremented unsigned (1 ... N)
where factor is:
b for byte (*1)
h for halfwords (*2)
w for words (*4)
d for doublewords (*8) -- useful for 64-bit MIPS16 but probably not
needed for 32-bit microMIPS
and where bits is the number of bits. type and factor could be
replaced with an ad-hoc two-letter combination for special cases.
E.g. Uas9 (add stack) for ADDISUP.
Just a suggestion though. I'm not saying these names are totally
intuitive or anything, but they should at least be better than arbitrary
letters.
Also, bits could be two digits if necessary, or we could just use hex
digits.
I extended this proposal a bit by:
1. Adding a type e for encoded. The constraint will start with Ue,
when the operand is an encoded value.
2. I decided to use two digits for bits.
3. The ad-hoc combination is used for anything else.
First of all, thanks for coming up with a counter-suggestion. I'm hopeless
at
naming things, so I was hoping there would be at least some pushback.
e for encoded sounds good. I'm less keen on the mixture of single- and
double-digit widths though (single digit for some Ues, double digits for
other Us.) I think we should either:
(a) use the same number of digits for all U constraints. That leaves
one character for the Ue type, which isn't as mnemonic, but is in
line with what we do elsewhere.
(b) avoid digits in the Ue forms and just have an ad-hoc letter
combination.
FWIW, as far as this point goes, the patch still has Uea4.
+/* Return true if X is a legitimate address that conforms to the
requirements
+ for any of the short microMIPS LOAD or STORE instructions except LWSP
+ or SWSP. */
+
+bool
+umips_address_p (rtx x, bool load, enum machine_mode mode) {
+
+ struct mips_address_info addr;
+ bool ok = mips_classify_address (addr, x, mode, false)
+ addr.type == ADDRESS_REG
+ M16_REG_P (REGNO (addr.reg))
+ CONST_INT_P (addr.offset);
+
+ if (!ok)
+return false;
+
+ if (mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2)
+ || mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 1))
+return true;
+
+ if (load IN_RANGE (INTVAL (addr.offset), -1, 14))
+return true;
+
+ if (!load IN_RANGE (INTVAL (addr.offset), 0, 15))
+return true;
+
+ return false;
+
+}
No blank lines after { and before }.
More importantly, what cases are these range tests covering?
Both binutils and qemu seem to think that LW and SW need offsets that
exactly match:
mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2)
Those range tests are for the LBU16 and SB16 instructions. LBU16
supports offsets from -1 to 14 (encodes -1 as 15) while the SB16 insn
supports offsets from 0-15.
They need to use separate constraints in that case. The patch as written
allows -1 offsets for LW16 too. (In rare cases, offsets like -1 can be
used even with aligned word loads.)
E.g. we could have:
/* Return true if X is legitimate for accessing values of mode MODE,
if it is based on a MIPS16 register, and if the offset satisfies
OFFSET_PREDICATE. */
bool
m16_based_address_p (rtx x, enum machine_mode mode,
insn_operand_predicate_fn predicate)
{
struct mips_address_info addr;
return (mips_classify_address (addr, x, mode, false)
addr.type == ADDRESS_REG
M16_REG_P (REGNO (addr.reg))
offset_predicate (addr.offset));
}
Perhaps if there are so many of these, we should define T??? to be
a memory constraint in which the base register is an M16_REG and in
which ??? has the same format as for U. E.g. Tuw4 for LW and SW.
That's just a suggestion though.
Thanks,
Richard
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Tuesday, March 19, 2013 3:26 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Tuesday, March 05, 2013 4:06 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support:
We have a few internal-only undocumented constraints that aren't
used much, so we should be able to move them to the Y space
instead.
The patch below does this for T and U. Then we could use U
for new, longer constraints.
Utypefactorbits
where type is:
s for signed
u for unsigned
d for decremented unsigned (-1 ... N)
i for incremented unsigned (1 ... N)
where factor is:
b for byte (*1)
h for halfwords (*2)
w for words (*4)
d for doublewords (*8) -- useful for 64-bit MIPS16 but probably not
needed for 32-bit microMIPS
and where bits is the number of bits. type and factor could
be replaced with an ad-hoc two-letter combination for special cases.
E.g. Uas9 (add stack) for ADDISUP.
Just a suggestion though. I'm not saying these names are totally
intuitive or anything, but they should at least be better than
arbitrary
letters.
Also, bits could be two digits if necessary, or we could just
use hex
digits.
I extended this proposal a bit by:
1. Adding a type e for encoded. The constraint will start with
Ue, when the operand is an encoded value.
2. I decided to use two digits for bits.
3. The ad-hoc combination is used for anything else.
First of all, thanks for coming up with a counter-suggestion. I'm
hopeless at naming things, so I was hoping there would be at least some
pushback.
e for encoded sounds good. I'm less keen on the mixture of
single- and double-digit widths though (single digit for some Ues,
double digits for other Us.) I think we should either:
(a) use the same number of digits for all U constraints. That leaves
one character for the Ue type, which isn't as mnemonic, but is in
line with what we do elsewhere.
(b) avoid digits in the Ue forms and just have an ad-hoc letter
combination.
FWIW, as far as this point goes, the patch still has Uea4.
+/* Return true if X is a legitimate address that conforms to the
requirements
+ for any of the short microMIPS LOAD or STORE instructions except
LWSP
+ or SWSP. */
+
+bool
+umips_address_p (rtx x, bool load, enum machine_mode mode) {
+
+ struct mips_address_info addr;
+ bool ok = mips_classify_address (addr, x, mode, false)
+addr.type == ADDRESS_REG
+M16_REG_P (REGNO (addr.reg))
+CONST_INT_P (addr.offset);
+
+ if (!ok)
+return false;
+
+ if (mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2)
+ || mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 1))
+return true;
+
+ if (load IN_RANGE (INTVAL (addr.offset), -1, 14))
+return true;
+
+ if (!load IN_RANGE (INTVAL (addr.offset), 0, 15))
+return true;
+
+ return false;
+
+}
No blank lines after { and before }.
More importantly, what cases are these range tests covering?
Both binutils and qemu seem to think that LW and SW need offsets that
exactly match:
mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2)
Those range tests are for the LBU16 and SB16 instructions. LBU16
supports offsets from -1 to 14 (encodes -1 as 15) while the SB16 insn
supports offsets from 0-15.
They need to use separate constraints in that case. The patch as written
allows -1 offsets for LW16 too. (In rare cases, offsets like -1 can be used
even
with aligned word loads.)
E.g. we could have:
/* Return true if X is legitimate for accessing values of mode MODE,
if it is based on a MIPS16 register, and if the offset satisfies
OFFSET_PREDICATE. */
bool
m16_based_address_p (rtx x, enum machine_mode mode,
insn_operand_predicate_fn predicate) {
struct mips_address_info addr;
return (mips_classify_address (addr, x, mode, false)
addr.type == ADDRESS_REG
M16_REG_P (REGNO (addr.reg))
offset_predicate (addr.offset));
}
Perhaps if there are so many of these, we should define T??? to be a
memory constraint in which the base register is an M16_REG and in which
??? has the same format as for U. E.g. Tuw4 for LW and SW.
That's just a suggestion though.
I'd just as soon leave them as Z constraints, if that's okay with you.
Yeah, that's fine.
I do see the need for separate
RE: FW: [PATCH] [MIPS] microMIPS gcc support
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Tuesday, March 19, 2013 3:26 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Tuesday, March 05, 2013 4:06 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support:
We have a few internal-only undocumented constraints that aren't
used much, so we should be able to move them to the Y space
instead.
The patch below does this for T and U. Then we could use U
for new, longer constraints.
Utypefactorbits
where type is:
s for signed
u for unsigned
d for decremented unsigned (-1 ... N)
i for incremented unsigned (1 ... N)
where factor is:
b for byte (*1)
h for halfwords (*2)
w for words (*4)
d for doublewords (*8) -- useful for 64-bit MIPS16 but probably not
needed for 32-bit microMIPS
and where bits is the number of bits. type and factor could
be replaced with an ad-hoc two-letter combination for special cases.
E.g. Uas9 (add stack) for ADDISUP.
Just a suggestion though. I'm not saying these names are totally
intuitive or anything, but they should at least be better than
arbitrary
letters.
Also, bits could be two digits if necessary, or we could just
use hex
digits.
I extended this proposal a bit by:
1. Adding a type e for encoded. The constraint will start with
Ue, when the operand is an encoded value.
2. I decided to use two digits for bits.
3. The ad-hoc combination is used for anything else.
First of all, thanks for coming up with a counter-suggestion. I'm
hopeless at naming things, so I was hoping there would be at least some
pushback.
e for encoded sounds good. I'm less keen on the mixture of
single- and double-digit widths though (single digit for some Ues,
double digits for other Us.) I think we should either:
(a) use the same number of digits for all U constraints. That leaves
one character for the Ue type, which isn't as mnemonic, but is in
line with what we do elsewhere.
(b) avoid digits in the Ue forms and just have an ad-hoc letter
combination.
FWIW, as far as this point goes, the patch still has Uea4.
+/* Return true if X is a legitimate address that conforms to the
requirements
+ for any of the short microMIPS LOAD or STORE instructions except
LWSP
+ or SWSP. */
+
+bool
+umips_address_p (rtx x, bool load, enum machine_mode mode) {
+
+ struct mips_address_info addr;
+ bool ok = mips_classify_address (addr, x, mode, false)
+ addr.type == ADDRESS_REG
+ M16_REG_P (REGNO (addr.reg))
+ CONST_INT_P (addr.offset);
+
+ if (!ok)
+return false;
+
+ if (mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2)
+ || mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 1))
+return true;
+
+ if (load IN_RANGE (INTVAL (addr.offset), -1, 14))
+return true;
+
+ if (!load IN_RANGE (INTVAL (addr.offset), 0, 15))
+return true;
+
+ return false;
+
+}
No blank lines after { and before }.
More importantly, what cases are these range tests covering?
Both binutils and qemu seem to think that LW and SW need offsets that
exactly match:
mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2)
Those range tests are for the LBU16 and SB16 instructions. LBU16
supports offsets from -1 to 14 (encodes -1 as 15) while the SB16 insn
supports offsets from 0-15.
They need to use separate constraints in that case. The patch as written
allows -1 offsets for LW16 too. (In rare cases, offsets like -1 can be used
even
with aligned word loads.)
E.g. we could have:
/* Return true if X is legitimate for accessing values of mode MODE,
if it is based on a MIPS16 register, and if the offset satisfies
OFFSET_PREDICATE. */
bool
m16_based_address_p (rtx x, enum machine_mode mode,
insn_operand_predicate_fn predicate) {
struct mips_address_info addr;
return (mips_classify_address (addr, x, mode, false)
addr.type == ADDRESS_REG
M16_REG_P (REGNO (addr.reg))
offset_predicate (addr.offset));
}
Perhaps if there are so many of these, we should define T??? to be a
memory constraint in which the base register is an M16_REG and in which
??? has the same format as for U. E.g. Tuw4 for LW and SW.
That's just a suggestion though.
I'd just as soon leave them as Z constraints, if that's okay with you. I do
see the need for separate constraints. I'll fix that up.
Adding the new microMIPS memory
RE: FW: [PATCH] [MIPS] microMIPS gcc support
Hi Richard, There are a couple of embedded comments, plus new patch attached. Are we there yet? Thanks, Catherine -Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Thursday, March 14, 2013 4:55 PM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support Moore, Catherine catherine_mo...@mentor.com writes: -Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Tuesday, March 05, 2013 4:06 PM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support: We have a few internal-only undocumented constraints that aren't used much, so we should be able to move them to the Y space instead. The patch below does this for T and U. Then we could use U for new, longer constraints. Utypefactorbits where type is: s for signed u for unsigned d for decremented unsigned (-1 ... N) i for incremented unsigned (1 ... N) where factor is: b for byte (*1) h for halfwords (*2) w for words (*4) d for doublewords (*8) -- useful for 64-bit MIPS16 but probably not needed for 32-bit microMIPS and where bits is the number of bits. type and factor could be replaced with an ad-hoc two-letter combination for special cases. E.g. Uas9 (add stack) for ADDISUP. Just a suggestion though. I'm not saying these names are totally intuitive or anything, but they should at least be better than arbitrary letters. Also, bits could be two digits if necessary, or we could just use hex digits. I extended this proposal a bit by: 1. Adding a type e for encoded. The constraint will start with Ue, when the operand is an encoded value. 2. I decided to use two digits for bits. 3. The ad-hoc combination is used for anything else. First of all, thanks for coming up with a counter-suggestion. I'm hopeless at naming things, so I was hoping there would be at least some pushback. e for encoded sounds good. I'm less keen on the mixture of single- and double-digit widths though (single digit for some Ues, double digits for other Us.) I think we should either: (a) use the same number of digits for all U constraints. That leaves one character for the Ue type, which isn't as mnemonic, but is in line with what we do elsewhere. (b) avoid digits in the Ue forms and just have an ad-hoc letter combination. Please keep U for constants though. The memory constraints should go under Z instead (and therefore be only two letters long). The idea is that the first letter of the constraint tells you what type it is. I don't think there's any need for the Ue constraints to have predicates of the same name. We can go with longer, mnemonic, names instead. The idea behind suggesting sb4_operand, etc., was that (a) every character was predictable and (b) I'm not sure the extra verbosity of (say) signed_byte_4_operand would help. But addiur2_operand would be good. +(define_constraint Udb07 + @internal + A decremented unsigned constant of 7 bits. + (match_operand 0 Udb07_operand )) Very minor nit, but these are redundant. +(define_constraint Ueim4 + @internal + A microMIPS encoded ADDIUR2 immediate operand. + (match_operand 0 Ueim4_operand )) Again minor, but the name doesn't really seem to match the description. Is this constraint needed for things other than ADDIUR2? The constraint is only used for ADDIUR2. If so, it might be worth giving a second example, otherwise it might be better to make the name a bit less general. Unless this name comes from the manual, of course :-) (The microMIPS link on the MIPS website was still broken last time I checked, but I haven't tried it again in the last couple of weeks.) +(define_predicate Umem0_operand + (and (match_code mem) + (match_test umips_lwsp_swsp_address_p (XEXP (op, 0), +mode + +(define_predicate Uload_operand + (and (match_code mem) + (match_test umips_address_p (XEXP (op, 0), true, mode + +(define_predicate Ustore_operand + (and (match_code mem) + (match_test umips_address_p (XEXP (op, 0), false, mode With the two-letter Z constraints, these should have descriptive names. +(define_predicate Udb07_operand + (and (match_code const_int) + (match_test mips_unsigned_immediate_p (INTVAL (op) + 1, 7, +0 Please drop the Us in the predicate names. +(define_attr compression none,all,micromips,mips16 + (const_string none)) Thinking about it a bit more, it would probably be better to leave the all and mips16 values out until we need them. You probably missed it, but there is one use of the all constraint in the patch. It's the first alternative in the *movmode_internal pattern. I've removed the mips16 value in the current patch
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes: -Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Tuesday, March 05, 2013 4:06 PM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support: We have a few internal-only undocumented constraints that aren't used much, so we should be able to move them to the Y space instead. The patch below does this for T and U. Then we could use U for new, longer constraints. Utypefactorbits where type is: s for signed u for unsigned d for decremented unsigned (-1 ... N) i for incremented unsigned (1 ... N) where factor is: b for byte (*1) h for halfwords (*2) w for words (*4) d for doublewords (*8) -- useful for 64-bit MIPS16 but probably not needed for 32-bit microMIPS and where bits is the number of bits. type and factor could be replaced with an ad-hoc two-letter combination for special cases. E.g. Uas9 (add stack) for ADDISUP. Just a suggestion though. I'm not saying these names are totally intuitive or anything, but they should at least be better than arbitrary letters. Also, bits could be two digits if necessary, or we could just use hex digits. I extended this proposal a bit by: 1. Adding a type e for encoded. The constraint will start with Ue, when the operand is an encoded value. 2. I decided to use two digits for bits. 3. The ad-hoc combination is used for anything else. First of all, thanks for coming up with a counter-suggestion. I'm hopeless at naming things, so I was hoping there would be at least some pushback. e for encoded sounds good. I'm less keen on the mixture of single- and double-digit widths though (single digit for some Ues, double digits for other Us.) I think we should either: (a) use the same number of digits for all U constraints. That leaves one character for the Ue type, which isn't as mnemonic, but is in line with what we do elsewhere. (b) avoid digits in the Ue forms and just have an ad-hoc letter combination. Please keep U for constants though. The memory constraints should go under Z instead (and therefore be only two letters long). The idea is that the first letter of the constraint tells you what type it is. I don't think there's any need for the Ue constraints to have predicates of the same name. We can go with longer, mnemonic, names instead. The idea behind suggesting sb4_operand, etc., was that (a) every character was predictable and (b) I'm not sure the extra verbosity of (say) signed_byte_4_operand would help. But addiur2_operand would be good. +(define_constraint Udb07 + @internal + A decremented unsigned constant of 7 bits. + (match_operand 0 Udb07_operand )) Very minor nit, but these are redundant. +(define_constraint Ueim4 + @internal + A microMIPS encoded ADDIUR2 immediate operand. + (match_operand 0 Ueim4_operand )) Again minor, but the name doesn't really seem to match the description. Is this constraint needed for things other than ADDIUR2? If so, it might be worth giving a second example, otherwise it might be better to make the name a bit less general. Unless this name comes from the manual, of course :-) (The microMIPS link on the MIPS website was still broken last time I checked, but I haven't tried it again in the last couple of weeks.) +(define_predicate Umem0_operand + (and (match_code mem) + (match_test umips_lwsp_swsp_address_p (XEXP (op, 0), mode + +(define_predicate Uload_operand + (and (match_code mem) + (match_test umips_address_p (XEXP (op, 0), true, mode + +(define_predicate Ustore_operand + (and (match_code mem) + (match_test umips_address_p (XEXP (op, 0), false, mode With the two-letter Z constraints, these should have descriptive names. +(define_predicate Udb07_operand + (and (match_code const_int) + (match_test mips_unsigned_immediate_p (INTVAL (op) + 1, 7, 0 Please drop the Us in the predicate names. +(define_attr compression none,all,micromips,mips16 + (const_string none)) Thinking about it a bit more, it would probably be better to leave the all and mips16 values out until we need them. +(define_attr enabled no,yes + (if_then_else (ior (eq_attr compression all) + (eq_attr compression none) + (and (eq_attr compression micromips) + (match_test TARGET_MICROMIPS))) + (const_string yes) + (const_string no))) FWIW (probably not much after the above), it's also possible to write: (eq_attr compression all,none) @@ -5237,6 +5271,12 @@ return dinsn\t%0,%1,%2; } [(set_attr type shift) + (set_attr_alternative compression + [(if_then_else (ior (match_test GET_CODE (SET_SRC (PATTERN (insn))) == ASHIFT) + (match_test GET_CODE (SET_SRC (PATTERN (insn))) == LSHIFTRT
RE: FW: [PATCH] [MIPS] microMIPS gcc support
-Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Tuesday, March 05, 2013 4:06 PM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support: We have a few internal-only undocumented constraints that aren't used much, so we should be able to move them to the Y space instead. The patch below does this for T and U. Then we could use U for new, longer constraints. Utypefactorbits where type is: s for signed u for unsigned d for decremented unsigned (-1 ... N) i for incremented unsigned (1 ... N) where factor is: b for byte (*1) h for halfwords (*2) w for words (*4) d for doublewords (*8) -- useful for 64-bit MIPS16 but probably not needed for 32-bit microMIPS and where bits is the number of bits. type and factor could be replaced with an ad-hoc two-letter combination for special cases. E.g. Uas9 (add stack) for ADDISUP. Just a suggestion though. I'm not saying these names are totally intuitive or anything, but they should at least be better than arbitrary letters. Also, bits could be two digits if necessary, or we could just use hex digits. I extended this proposal a bit by: 1. Adding a type e for encoded. The constraint will start with Ue, when the operand is an encoded value. 2. I decided to use two digits for bits. 3. The ad-hoc combination is used for anything else. Patch is attached. WDYT? I'm still testing, but results so far look really good. Catherine short-delay.cl Description: short-delay.cl short-delay.patch Description: short-delay.patch
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Monday, March 04, 2013 3:54 PM
To: Moore, Catherine
Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
Hi Richard,
- Predicates should always check the code though. E.g.:
(define_predicate umips_addius5_imm
(and (match_code const_int)
(match_test IN_RANGE (INTVAL (op), -8, 7
- In general, please try to make the names of the predicates as generic
as possible. There's nothing really add-specific about the predicate
above. Or microMIPS-specific either really: some of these predicates
are probably going to be useful for MIPS16 too.
The existing MIPS16 functions follow the convention:
n if negated (optional)
+ s or u for signed vs. unsigned
+ imm
+ number of significant bits
+ _
+ multiplication factor or, er, b for +1...
It might be nice to have a similar convention for microMIPS.
The choices there are a bit more exotic, so please feel free to
diverge from the MIPS16 one above; we can switch MIPS16 over once
the microMIPS one is settled. In fact, a new convention that's
compact enough to be used in both predicate and constraint names
would be great. E.g. for the umips_addius5_imm predicate above,
a name like Ys5 would be easier to remember than Zo/Yo.
How compact would you consider compact enough? I would need to change
the existing Y constraints as well.
Argh, sorry, I'd forgotten about that restriction.
We have a few internal-only undocumented constraints that aren't used much,
so we should be able to move them to the Y space instead. The patch
below does this for T and U. Then we could use U for new, longer
constraints.
I think trying to invent some convention with less than four letter will
be difficult and even with four, I doubt it could be uniformly followed.
I think we could get descriptive with four, however.
Let me know what you think.
Four sounds good. Here's one idea:
Utypefactorbits
where type is:
s for signed
u for unsigned
d for decremented unsigned (-1 ... N)
i for incremented unsigned (1 ... N)
where factor is:
b for byte (*1)
h for halfwords (*2)
w for words (*4)
d for doublewords (*8) -- useful for 64-bit MIPS16 but probably not
needed for 32-bit microMIPS
and where bits is the number of bits. type and factor could be
replaced with an ad-hoc two-letter combination for special cases.
E.g. Uas9 (add stack) for ADDISUP.
Just a suggestion though. I'm not saying these names are totally intuitive
or anything, but they should at least be better than arbitrary letters.
Also, bits could be two digits if necessary, or we could just use
hex digits.
We could have:
/* Return true if X fits within an unsigned field of BITS bits that is
shifted left SHIFT bits before being used. */
static inline bool
mips_unsigned_immediate_p (unsigned HOST_WIDE_INT x, int bits, int shift = 0)
{
return (x ((1 shift) - 1)) == 0 x (1 (shift + bits));
}
/* Return true if X fits within a signed field of BITS bits that is
shifted left SHIFT bits before being used. */
static inline bool
mips_signed_immediate_p (unsigned HOST_WIDE_INT x, int bits, int shift = 0)
{
x += 1 (bits + shift - 1);
return mips_unsigned_immediate_p (x, bits, shift);
}
The 'd' and 'i' cases would pass a biased X to mips_unsigned_immediate_p.
I'll apply the patch below once 4.9 starts.
Thanks,
Richard
gcc/
* config/mips/constraints.md (T): Rename to...
(Yf): ...this.
(U): Rename to...
(Yd): ...this.
* config/mips/mips.md (*movdi_64bit, *movdi_64bit_mips16)
(*movmode_internal, *movmode_mips16): Update accordingly.
Index: gcc/config/mips/constraints.md
===
--- gcc/config/mips/constraints.md 2013-02-25 21:45:10.0 +
+++ gcc/config/mips/constraints.md 2013-03-05 08:22:36.687354771 +
@@ -170,22 +170,6 @@ (define_constraint S
(and (match_operand 0 call_insn_operand)
(match_test CONSTANT_P (op
-(define_constraint T
- @internal
- A constant @code{move_operand} that cannot be safely loaded into @code{$25}
- using @code{la}.
- (and (match_operand 0 move_operand)
- (match_test CONSTANT_P (op))
- (match_test mips_dangerous_for_la25_p (op
-
-(define_constraint U
- @internal
- A constant @code{move_operand} that can be safely loaded into @code{$25}
- using @code{la}.
- (and (match_operand 0 move_operand)
- (match_test CONSTANT_P (op))
- (not (match_test mips_dangerous_for_la25_p (op)
-
(define_memory_constraint W
@internal
A memory address based on a member of @code{BASE_REG_CLASS}. This is
@@ -220,6 +204,22 @@ (define_constraint Yb
@internal
RE: FW: [PATCH] [MIPS] microMIPS gcc support
Hi Richard,
I've attached an example patch for the add pattern that tries to identify the
short microMIPS variants. In your last review, you mentioned that you would
like to see the register requirements modeled in the patterns. Do you have any
comments on the approach that I'm taking? Thanks,
Catherine
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@googlemail.com]
Sent: Thursday, July 19, 2012 8:46 PM
-
+(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)))
-
+/* 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.
Richard
short-delay.cl
Description: short-delay.cl
short-delay.patch
Description: short-delay.patch
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes: Hi Richard, I've attached an example patch for the add pattern that tries to identify the short microMIPS variants. In your last review, you mentioned that you would like to see the register requirements modeled in the patterns. Do you have any comments on the approach that I'm taking? Thanks, Looks good, thanks. I was a bit surprised that we want to hide this completely from the register allocators (by adding ! to all of the micromips forms) but I can see that being too honest about the alternatives might lead to poor decisions. That's definitely something that could be retuned later if someone wants to. A few bikesheddy comments: - Please use Y... constraints for constants (as with Yb, Yh, etc.). I was hoping to keep Y... for constants and Z... for memory stuff. - You used a mixture of predicates and out-of-line functions to do the matching. I think we should use the predicate approach across the board, because it's hard to predict which ones will come in useful in future. - Predicates should always check the code though. E.g.: (define_predicate umips_addius5_imm (and (match_code const_int) (match_test IN_RANGE (INTVAL (op), -8, 7 - In general, please try to make the names of the predicates as generic as possible. There's nothing really add-specific about the predicate above. Or microMIPS-specific either really: some of these predicates are probably going to be useful for MIPS16 too. The existing MIPS16 functions follow the convention: n if negated (optional) + s or u for signed vs. unsigned + imm + number of significant bits + _ + multiplication factor or, er, b for +1... It might be nice to have a similar convention for microMIPS. The choices there are a bit more exotic, so please feel free to diverge from the MIPS16 one above; we can switch MIPS16 over once the microMIPS one is settled. In fact, a new convention that's compact enough to be used in both predicate and constraint names would be great. E.g. for the umips_addius5_imm predicate above, a name like Ys5 would be easier to remember than Zo/Yo. That said，I realise things like umips_addiur2_imm_p are a bit hard to describe this way and might need to stay instruction-specific. - We already have a ks constraint for the stack pointer. - I like the choice of u for M16_REGS. Kind of lucky that that letter's still free. :-) As you've probably noticed though, we're running desparately short of contraint letters, so if you also need names for other less frequently-used classes, it might be better to use k... for those. Thanks, Richard
RE: FW: [PATCH] [MIPS] microMIPS gcc support
-Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Monday, March 04, 2013 3:54 PM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support Moore, Catherine catherine_mo...@mentor.com writes: Hi Richard, - Predicates should always check the code though. E.g.: (define_predicate umips_addius5_imm (and (match_code const_int) (match_test IN_RANGE (INTVAL (op), -8, 7 - In general, please try to make the names of the predicates as generic as possible. There's nothing really add-specific about the predicate above. Or microMIPS-specific either really: some of these predicates are probably going to be useful for MIPS16 too. The existing MIPS16 functions follow the convention: n if negated (optional) + s or u for signed vs. unsigned + imm + number of significant bits + _ + multiplication factor or, er, b for +1... It might be nice to have a similar convention for microMIPS. The choices there are a bit more exotic, so please feel free to diverge from the MIPS16 one above; we can switch MIPS16 over once the microMIPS one is settled. In fact, a new convention that's compact enough to be used in both predicate and constraint names would be great. E.g. for the umips_addius5_imm predicate above, a name like Ys5 would be easier to remember than Zo/Yo. How compact would you consider compact enough? I would need to change the existing Y constraints as well. Would YG -- Yvec0 be too long? I think trying to invent some convention with less than four letter will be difficult and even with four, I doubt it could be uniformly followed. I think we could get descriptive with four, however. Let me know what you think. That said，I realise things like umips_addiur2_imm_p are a bit hard to describe this way and might need to stay instruction-specific. - We already have a ks constraint for the stack pointer. - I like the choice of u for M16_REGS. Kind of lucky that that letter's still free. :-) As you've probably noticed though, we're running desparately short of contraint letters, so if you also need names for other less frequently-used classes, it might be better to use k... for those. Thanks, Richard
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes: HI Richard, The base patch is now committed. The final patch, including your final edits is attached. I will be posting the optimization pieces later this week. Sorry, when I said that it was ok for 4.9, I meant that it should wait until 4.8 had branched. As I say, I think the patch is too invasive for 4.8 at this (regression fixes only) stage. Please revert the patch for now and reapply it once 4.9 starts. Richard
RE: FW: [PATCH] [MIPS] microMIPS gcc support
-Original Message- From: Richard Sandiford [mailto:rdsandif...@googlemail.com] Sent: Monday, February 25, 2013 4:42 AM To: Moore, Catherine Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support Moore, Catherine catherine_mo...@mentor.com writes: HI Richard, The base patch is now committed. The final patch, including your final edits is attached. I will be posting the optimization pieces later this week. Sorry, when I said that it was ok for 4.9, I meant that it should wait until 4.8 had branched. As I say, I think the patch is too invasive for 4.8 at this (regression fixes only) stage. Please revert the patch for now and reapply it once 4.9 starts. Now reverted. I thought that we had branched. Catherine
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
Looks good otherwise, but please post an updated patch. It's probably
stating the obvious, but the patch is too invasive for this late stage
of 4.8, so
it'll need to wait until 4.9.
The updated patch is attached. How's it looking this time?
Almost there. :-)
As I mentioned in an earlier reply, please always check for warnings.
If you're not bootstrapping on a MIPS host then instead configure the
cross compiler with --enable-werror-always, using a recent GCC as the
host compiler. If the patch goes in with warnings, it'll break the
build for other people.
The warnings I could see are:
+ for (n = 0; n XVECLEN (pattern, 0); n++)
+{
+ rtx set, reg, mem, this_base;
+ HOST_WIDE_INT this_offset;
+ unsigned int this_regno;
this_regno is an unused variable (thanks for making it unused though).
static bool
mips_function_ok_for_sibcall (tree decl, tree exp ATTRIBUTE_UNUSED)
{
+
+ unsigned int compression_mode = mips_get_compress_mode (decl);
compression_mode is unused.
+static bool
+umips_build_save_restore (mips_save_restore_fn fn,
+ unsigned *mask, HOST_WIDE_INT *offset)
+{
+ int i, nregs;
+ unsigned int j;
+ rtx pattern, set, reg, mem;
+ HOST_WIDE_INT this_offset;
+ rtx this_base;
+
+ /* Try matching $16 to $31 (s0 to ra). */
+ for (i = 0; i ARRAY_SIZE (umips_swm_mask); i++)
+if ((*mask 0x) == umips_swm_mask[i])
+ break;
+
+ if (i == ARRAY_SIZE (umips_swm_mask))
+return false;
i needs to be unsigned to avoid a comparison between signed and unsigned
warning.
+/* 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));
+
+ s = strcpy (buffer, save_p ? swm\t : lwm\t);
+ s += strlen (s);
+ n = XVECLEN (pattern, 0);
+
+ set = XVECEXP (pattern, 0, 0);
+ reg = save_p ? SET_SRC (set) : SET_DEST (set);
reg is unused and should be deleted.
In case it sounds otherwise, I didn't build the patch to try to catch you out.
I built it so that I could try to answer:
Is this a test of the swap_p case? Thanks if so, but could you add a
comment
saying where the SWP gets generated, and why the test needs to be skipped
at -O1 and -Os?
Yes, this was the swap_p case. But it turns out that umips-lpw-swp-1.c
was also testing the swap_p case. I've now dropped this test in favor
of a different test that exercises the swap_p = false case.
You had mentioned testing for explicit registers. This is turning out
to be problematic. The Linux and ELF tool chains don't always use the
same register set. In addition, -O3 seems to expose more opportunities
for generating these instructions so the code generated isn't consistent
among optimization options. I've changed it back to just testing for
swp, but I'm open to other suggestions.
I've attached some tests below, both for LWP (which didn't seem to be
covered) and SWP. They use things like multiplication to force a
particular load or store order in scheduled code, so that both swap_p
and !swap_p are covered (verified using both printfs and -fno-peephole2).
umips-lwp-7.c tests for:
/* Avoid invalid load pair instructions. */
if (load_p REGNO (first_reg) == REGNO (base1))
return false;
/* We must avoid this case for anti-dependence.
Ex: lw $3, 4($3)
lw $2, 0($3)
first_reg is $2, but the base is $3. */
if (load_p
swap_p
REGNO (first_reg) + 1 == REGNO (base1))
return false;
while umips-lwp-8.c is an example of something that would be pessimised
by not having the swap_p test above.
I also noticed a couple of other things while testing:
- ISA_HAS_LOAD_DELAY should be false for microMIPS, otherwise we'll try
to insert nops after LWP on targets that default to -mips1, and ICE
when we see that we have a double SET.
- In the testsuite, a -mmicromips test option should override an ambient
-mips16, and vice versa.
- umips-movep-2.c fails on n64 because there the moves are DImode rather
than SImode. Since the initial patch is just for 32-bit, I forced
-mgp32 for now. Also, the function needs a MICROMIPS attribute.
There were various other minor formatting and comment nits that were probably
easier for me to fix than describe. There was also some trailing whitespace.
The patch is OK for 4.9 with this one applied on top of it, provided that
it works with your setup of course. Thanks for your patience!
Richard
Index: gcc/config/mips/constraints.md
===
--- gcc/config/mips/constraints.md
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Thanks, this is looking much better now.
Moore, Catherine catherine_mo...@mentor.com writes:
I'd also like to support -mno-jals for backward compatibility. Are you
okay with that? If so, I'll submit as a separate patch.
Hmm, to be honest, I'd rather you kept it local to the Mentor toolchain.
The way the patch is written, we (rightly) use JALS even with
-minterlink-compressed in cases where we can prove that the call
is to another microMIPS function, so calling -mno-jals an alias of
-minterlink-compressed would be a bit confusing.
-mno-jals sounds like it ought to disable JALS in all situations,
which would need special code to handle. I think doing that would be
extra baggage with little benefit.
Index: gcc.target/mips/umips-constraints-1.c
===
--- gcc.target/mips/umips-constraints-1.c (revision 0)
+++ gcc.target/mips/umips-constraints-1.c (revision 0)
@@ -0,0 +1,14 @@
+/* { dg-options (-mmicromips) } */
+/* { dg-skip-if code quality test { *-*-* } { -O0 } { } } */
+
+MICROMIPS void
+foo (int *x)
+{
+ asm volatile (insn1\t%a0 :: ZD (x[0]));
+ asm volatile (insn2\t%a0 :: ZD (x[511]));
+ asm volatile (insn3\t%a0 :: ZD (x[512]));
+}
+
+/* { dg-final { scan-assembler \tinsn1\t0\\( } } */
+/* { dg-final { scan-assembler \tinsn2\t2044\\( } } */
+/* { dg-final { scan-assembler \tinsn3\t2048\\( } } */
But the insn3 is wrong, isn't it? I suggested scan-assembler-not for
that one because I thought it had to be a 12-bit signed offset on
microMIPS. The patch has:
(define_address_constraint ZD
When compiling microMIPS code, this constraint matches an address operand
that is formed from a base register and a 12-bit offset. These operands
can be used for microMIPS instructions such as @code{prefetch}. When
not compiling for microMIPS code, @code{ZD} is equivalent to @code{p}.
(ior (and (match_test TARGET_MICROMIPS)
(match_test umips_12bit_offset_address_p (op, mode)))
(match_test mips_address_insns (op, mode, false
but it looks like it ought to be:
(if_then_else (match_test TARGET_MICROMIPS)
(match_test umips_12bit_offset_address_p (op, mode))
(match_test mips_address_insns (op, mode, false)))
Same for ZC.
@@ -1246,6 +1247,10 @@ proc mips-dg-options { args } {
append extra_tool_flags -DMIPS16=__attribute__((mips16))
}
+if { [mips_have_test_option_p options -mmicromips] } {
+ append extra_tool_flags -DMICROMIPS=__attribute__((micromips))
+}
+
# Use our version of gcc-dg-test for this test.
if { ![string equal [info procs mips-gcc-dg-test] ] } {
rename gcc-dg-test mips-old-gcc-dg-test
@@ -1275,6 +1280,6 @@ proc mips-gcc-dg-test { prog do_what extra_tool_fl
dg-init
mips-dg-init
gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] \
--DNOMIPS16=__attribute__((nomips16))
+-DNOMIPS16=__attribute__((nomips16))
-DNOMICROMIPS=__attribute__((nomicromips))
-DNOCOMPRESSION=__attribute__((nocompression))
-DMICROMIPS=__attribute__((micromips))
mips-dg-finish
dg-finish
Please drop the -DMICROMIPS in the second hunk. The first hunk is the
right way to add it. The idea is that we want a compilation failure if
the test uses MICROMIPS but forgets to add:
/* { dg-options (-mmicromips) } */
The -DNOMICROMIPS and -DNOCOMPRESSION in the second hunk are fine though.
+/* { dg-options (-mmicromips) } */
+/* { dg-skip-if code quality test { *-*-* } { -O0 -O1 } { } } */
Sorry, only just realised you were skipping -O1. Please add a comment saying
why it fails at -O1 or (preferably) add whichever other option is needed for
the test to pass at -O1. I assume it's -fpeephole2 in this case.
Same for all tests with this skip.
Index: gcc.target/mips/umips-lwp-swp-2.c
===
--- gcc.target/mips/umips-lwp-swp-2.c (revision 0)
+++ gcc.target/mips/umips-lwp-swp-2.c (revision 0)
@@ -0,0 +1,19 @@
+/* { dg-options -mgp32 (-mmicromips) } */
+/* { dg-skip-if code quality test { *-*-* } { -O0 -O1 -Os } { } }
*/
+int a[2];
+
+MICROMIPS f (b)
+{
+ unsigned int i;
+ int *p;
+ for (p = a[b], i = b; --i ~0; )
+*--p = i * 3 + (int)a;
+
+}
+
+MICROMIPS main ()
+{
+ a[0] = a[1] = 0;
+ f (2);
+}
+/* { dg-final { scan-assembler \tswp\t\\\$3,0\\(\\\$3\\) } }*/
Is this a test of the swap_p case? Thanks if so, but could you add a
comment saying where the SWP gets generated, and why the test needs
to be skipped at -O1 and -Os?
Index: gcc.target/mips/umips-lwp-swp-volatile.c
===
--- gcc.target/mips/umips-lwp-swp-volatile.c (revision 0)
+++ gcc.target/mips/umips-lwp-swp-volatile.c (revision 0)
@@ -0,0 +1,41 @@
+/* { dg-do compile } */
+
+/* This test ensures that we do not generate microMIPS SWP or LWP
+ instructions
Re: FW: [PATCH] [MIPS] microMIPS gcc support
On Tue, 19 Feb 2013, Richard Sandiford wrote:
Index: gcc.target/mips/umips-lwp-swp-2.c
===
--- gcc.target/mips/umips-lwp-swp-2.c (revision 0)
+++ gcc.target/mips/umips-lwp-swp-2.c (revision 0)
@@ -0,0 +1,19 @@
+/* { dg-options -mgp32 (-mmicromips) } */
+/* { dg-skip-if code quality test { *-*-* } { -O0 -O1 -Os } { }
} */
+int a[2];
+
+MICROMIPS f (b)
+{
+ unsigned int i;
+ int *p;
+ for (p = a[b], i = b; --i ~0; )
+*--p = i * 3 + (int)a;
+
+}
+
+MICROMIPS main ()
+{
+ a[0] = a[1] = 0;
+ f (2);
+}
+/* { dg-final { scan-assembler \tswp\t\\\$3,0\\(\\\$3\\) } }*/
Is this a test of the swap_p case? Thanks if so, but could you add a
comment saying where the SWP gets generated, and why the test needs
to be skipped at -O1 and -Os?
TBH, it is -Os where size really matters, so IMHO it is the -Os
optimisation level where LWP/SWP should be used where possible in the
first place, even if it hurts performance for some reason (and where e.g.
-O2 might decide to go for individual accesses instead).
Not to be meant as a show-stopper for this initial implementation, but if
the optimisation does not work for LWP/SWP at -Os for some reason, then I
think there's something going seriously wrong somewhere (instruction
lengths set wrong for example?) which looks to me worth investigating and
acting upon accordingly as the next step.
I saw cases with instruction lengths set too high with our trunk just
about yesterday and MIPS16 code, e.g.:
.file 1 foo.c
.section .mdebug.abi32
.previous
.gnu_attribute 4, 1
.abicalls
.option pic0
.text
.align 2
.globl foo
.setmips16
.entfoo
.type foo, @function
foo:
.frame $sp,32,$31 # vars= 0, regs= 1/0, args= 16, gp= 8
.mask 0x8000,-4
.fmask 0x,0
save32,$31 # 11 *mips16e_save_restore [length = 4]
.setnoreorder
.setnomacro
jal bar # 22 call_split/2[length = 8]
li $4,0 # 5*movsi_mips16/4 [length = 4]
.setmacro
.setreorder
restore 32,$31 # 18 *mips16e_save_restore [length = 4]
j $31 # 20 simple_return_internal [length = 2]
.endfoo
.size foo, .-foo
-- it looks all wrong to me except for the final J. Of course I realise
where it originally comes from, but I think it would be good if the
initial pessimistic estimates were actually adjusted as more is known
about actual insns produced.
I wonder if we may have similar issues with microMIPS instruction size
calculation triggering here.
Maciej
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@mentor.com writes:
Index: config/mips/mips.c
==
=
--- config/mips/mips.c (revision 195351)
+++ config/mips/mips.c (working copy)
@@ -77,6 +77,9 @@ along with GCC; see the file COPYING3. If not see
preserve the maximum stack alignment. We therefore use a value
of 0x7ff0 in this case.
+ microMIPS LWM and SWM support 12-bit offsets (from -0x800 to 0x7ff),
+ so we use a maximum of 0x7f0 for TARGET_MICROMIPS.
+
MIPS16e SAVE and RESTORE instructions can adjust the stack pointer by
up to 0x7f8 bytes and can usually save or restore all the registers
that we need to save or restore. (Note that we can only use these
@@ -87,7 +90,8 @@ along with GCC; see the file COPYING3. If not see
to save and restore registers, and to allocate and deallocate the top
part of the frame. */
#define MIPS_MAX_FIRST_STACK_STEP
\
- (!TARGET_MIPS16 ? 0x7ff0
\
+ ((!TARGET_MIPS16 !TARGET_MICROMIPS) ? 0x7ff0
\
+ : TARGET_MICROMIPS ? 0x7f0
\
: GENERATE_MIPS16E_SAVE_RESTORE ? 0x7f8
\
: TARGET_64BIT ? 0x100 : 0x400)
I'd prefer:
(TARGET_MICROMIPS ? 0x7f0 \
: !TARGET_MIPS16 ? 0x7ff0 \
: GENERATE_MIPS16E_SAVE_RESTORE ? 0x7f8 \
: TARGET_64BIT ? 0x100 : 0x400)
Thanks for doing this, but...
The comment doesn't really explain the reason for 0x7f0 rather than 0x7f8.
Is this in anticipation of future n32 and n64 support, where the stack must
be
16-byte aligned? If so, that's OK, but worth mentioning.
...it looks like you haven't addressed this part. What I mean is:
the stack alignment is 8 bytes rather than 16 bytes on 32-bit ABIs,
so I would have expected 0x7f8 rather than 0x7f0, as for the MIPS16
case quoted above. I wasn't sure why you went for 0x7f0 instead.
I'm not saying it must be 0x7f8, just that the comment ought to give
the reason for using 0x7f0 instead.
+; For movep
+(define_peephole2
+ [(set (match_operand:MOVEP1 0 register_operand )
+(match_operand:MOVEP1 1 movep_operand ))
+ (set (match_operand:MOVEP2 2 register_operand )
+(match_operand:MOVEP2 3 movep_operand ))]
+ TARGET_MICROMIPS
+umips_movep_target_p (operands[0], operands[2])
+ [(parallel [(set (match_dup 0) (match_dup 1))
+ (set (match_dup 2) (match_dup 3))])]
+)
Probably not worth having movep_operand, because all the checking is done
by umips_movep_target_p. reg_or_0_operand should be OK.
The source and target register criteria are not identical. I've now
renamed the movep_operand predicate to movep_src_operand.
Ah, yeah, sorry.
+ /* If DECL is nocompression set the nomips16 and
+ nomicromips attributes. */
+ if (nocompression_p MASK_MIPS16
+ nocompression_p MASK_MICROMIPS)
+ {
+name = nomips16;
+*attributes = tree_cons (get_identifier (name), NULL, *attributes);
+name = nomicromips;
+*attributes = tree_cons (get_identifier (name), NULL, *attributes);
+ }
Hmm, why is this necessary? Anything that cares should be using
mips_get_compress_off_flags.
It looks like you haven't addressed this part. Why do we need to add
tree-level nomips16 and nomicromips attributes to a function that
has nocompression?
+ addr.type == ADDRESS_REG
+ CONST_INT_P (addr.offset)
+ UMIPS_12BIT_OFFSET_P (addr.offset));
Isn't there a missing INTVAL here? Please check the patch to make sure
there are no compiler warnings.
This too. The point is that addr.offset is an rtx, while UMIPS_12BIT_OFFSET_P
takes a HOST_WIDE_INT:
#define UMIPS_12BIT_OFFSET_P(OFFSET) (IN_RANGE (OFFSET, -2048, 2047))
So you're comparing an rtx pointer against [-2048, 2047], rather than
the integer that the rtx contains.
As it stands, I don't see how umips_12bit_offset_address_p would ever
return true on normal hosts. That in turn should mean that ZC and ZD
never accept anything for microMIPS, so I would have expected this to
show up as a reload failure during the microMIPS testsuite run.
Please add tests that ZC and ZD work for microMIPS. E.g. the ZC
one could be something like:
MICROMIPS void
foo (int *x)
{
asm volatile (insn1\t%0 :: ZC (x[0]));
asm volatile (insn2\t%0 :: ZC (x[511]));
asm volatile (insn3\t%0 :: ZC (x[512]));
}
/* { dg-final { scan-assembler \tinsn1\t0\\( } } */
/* { dg-final { scan-assembler \tinsn2\t2044\\( } } */
/* { dg-final { scan-assembler-not \tinsn3\t2048\\( } } */
Completely untested :-) Similar idea for ZD.
+@item -minterlink-compressed
+@item -mno-interlink-compressed
+@item -minterlink-uncompressed
+@item -mno-interlink-uncompressed
+@opindex minterlink-compressed
+@opindex
Re: FW: [PATCH] [MIPS] microMIPS gcc support
On Sat, 26 Jan 2013, Richard Sandiford wrote: How about instead of complicating this we simply add support for microMIPS encoding in the PLT? I think I should be able to squeeze out some time next week to dust off and retest the binutils patch I've had pending far too long now. This way we won't have to maintain separate cases where tail calls may or may not be made via the PLT. Note that we need that support sooner or later anyway due to the prospect of pure-microMIPS processors. Just so I know: what does the PLT patch do for external functions that are jumped to by both microMIPS and non-microMIPS code? Two PLT entries are produced in that case. PLT entries are created based on the relocation type referring: R_MIPS_26 relocations trigger a standard MIPS PLT entry, R_MICROMIPS_26_S1 relocations trigger a microMIPS PLT entry. Other relocations reuse a PLT entry already produced for one of the jump relocations, or if none present, then they make an own PLT entry according to ELF file header flags: if EF_MIPS_ARCH_ASE_MICROMIPS is set, then a microMIPS entry is produced, otherwise a standard MIPS one. Therefore depending on relocations seen up to two entries can be produced, encoded differently so that there is no need to switch modes with direct jumps. If all the individual PLT entries ultimately produced are microMIPS code, then the PLT header is built as microMIPS code as well, otherwise it's standard MIPS code. This guarantees no standard MIPS code is produced in the PLT if there's none already in the executable (and vice versa). Maciej
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Maciej W. Rozycki ma...@codesourcery.com writes:
On Wed, 23 Jan 2013, Richard Sandiford wrote:
Index: config/mips/gnu-user.h
===
--- config/mips/gnu-user.h (revision 195351)
+++ config/mips/gnu-user.h (working copy)
@@ -137,3 +137,12 @@ extern const char *host_detect_local_cpu (int argc
#define ENDFILE_SPEC \
GNU_USER_TARGET_MATHFILE_SPEC \
GNU_USER_TARGET_ENDFILE_SPEC
+
+#undef SUBTARGET_OVERRIDE_OPTIONS
+#define SUBTARGET_OVERRIDE_OPTIONS \
+do {\
+ /* microMIPS PLT entries are non-microMIPS. */ \
+ TARGET_INTERLINK_COMPRESSED = 1; \
+} while (0)
+
+
Hmm, that sounds like a reason to set TARGET_INTERLINK_UNCOMPRESSED
rather than TARGET_INTERLINK_COMPRESSED. I.e. we need to avoid direct
branches from microMIPS code to standard PLTs.
But that means that microMIPS code can't even jump directly to functions
that have a micromips attribute when the call might go via a PLT.
TARGET_INTERLINK_(UN)COMPRESSED doesn't cover that case. I think instead
we need to handle it directly in mips_function_ok_for_sibcall,
keyed off TARGET_ABICALLS_PIC0. Specific suggestion below.
[...]
/* We can't do a sibcall if the called function is a MIPS16 function
because there is no direct jx instruction equivalent to jalx to
switch the ISA mode. We only care about cases where the sibling
and normal calls would both be direct. */
if (decl
- mips_use_mips16_mode_p (decl)
+ ((compression_mode (MASK_MIPS16 | MASK_MICROMIPS)) != 0)
const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode))
return false;
- /* When -minterlink-mips16 is in effect, assume that non-locally-binding
- functions could be MIPS16 ones unless an attribute explicitly tells
+ /* When -minterlink-compressed is in effect, assume that
non-locally-binding
+ functions could be MIPS16 or microMIPS unless an attribute
explicitly tells
us otherwise. */
- if (TARGET_INTERLINK_MIPS16
+ if (TARGET_INTERLINK_COMPRESSED
decl
(DECL_EXTERNAL (decl) || !targetm.binds_local_p (decl))
- !mips_nomips16_decl_p (decl)
+ (compression_mode MASK_MICROMIPS) == 0
+ (compression_mode MASK_MIPS16) == 0
const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode))
return false;
should be something like:
/* Direct Js are only possible to functions that use the same ISA encoding.
There is no JX counterpart of JALX. */
if (decl
const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode)
mips_call_may_need_jalx_p (decl))
return false;
with:
/* Return true if a call to DECL may need to use JALX. */
static bool
mips_call_may_need_jalx_p (decl)
{
/* If the current translation unit would use a different mode for DECL,
assume that the call needs JALX. */
if (mips_get_compress_mode (decl) != TARGET_COMPRESSION)
return true;
/* mips_get_compress_mode is always accurate for locally-binding
functions in the current translation unit. */
if (!DECL_EXTERNAL (decl) targetm.binds_local_p (decl))
return false;
/* PLTs use the standard encoding, so calls that might go via PLTs
must use JALX. */
if (TARGET_COMPRESSED
TARGET_ABICALLS_PIC0
DECL_EXTERNAL (decl)
!targetm.binds_local_p (decl))
return true;
/* When -minterlink-compressed is in effect, assume that functions
could use a different encoding mode unless an attribute explicitly
tells us otherwise. */
if (TARGET_INTERLINK_COMPRESSED)
{
if (!TARGET_COMPRESSION mips_get_compress_off_flags (decl) == 0)
return true;
if (TARGET_COMPRESSION mips_get_compress_on_flags (decl) == 0)
return true;
}
return false;
}
The TARGET_ABICALLS_PIC0 case should deal with the gnu-user.h comment above.
How about instead of complicating this we simply add support for
microMIPS encoding in the PLT? I think I should be able to squeeze out
some time next week to dust off and retest the binutils patch I've had
pending far too long now. This way we won't have to maintain separate
cases where tail calls may or may not be made via the PLT.
Note that we need that support sooner or later anyway due to the prospect
of pure-microMIPS processors.
Just so I know: what does the PLT patch do for external functions
that are jumped to by both microMIPS and non-microMIPS code?
Richard
Re: FW: [PATCH] [MIPS] microMIPS gcc support
On Wed, 23 Jan 2013, Richard Sandiford wrote:
Index: config/mips/gnu-user.h
===
--- config/mips/gnu-user.h (revision 195351)
+++ config/mips/gnu-user.h (working copy)
@@ -137,3 +137,12 @@ extern const char *host_detect_local_cpu (int argc
#define ENDFILE_SPEC \
GNU_USER_TARGET_MATHFILE_SPEC \
GNU_USER_TARGET_ENDFILE_SPEC
+
+#undef SUBTARGET_OVERRIDE_OPTIONS
+#define SUBTARGET_OVERRIDE_OPTIONS \
+do {\
+ /* microMIPS PLT entries are non-microMIPS. */ \
+ TARGET_INTERLINK_COMPRESSED = 1; \
+} while (0)
+
+
Hmm, that sounds like a reason to set TARGET_INTERLINK_UNCOMPRESSED
rather than TARGET_INTERLINK_COMPRESSED. I.e. we need to avoid direct
branches from microMIPS code to standard PLTs.
But that means that microMIPS code can't even jump directly to functions
that have a micromips attribute when the call might go via a PLT.
TARGET_INTERLINK_(UN)COMPRESSED doesn't cover that case. I think instead
we need to handle it directly in mips_function_ok_for_sibcall,
keyed off TARGET_ABICALLS_PIC0. Specific suggestion below.
[...]
/* We can't do a sibcall if the called function is a MIPS16 function
because there is no direct jx instruction equivalent to jalx to
switch the ISA mode. We only care about cases where the sibling
and normal calls would both be direct. */
if (decl
- mips_use_mips16_mode_p (decl)
+ ((compression_mode (MASK_MIPS16 | MASK_MICROMIPS)) != 0)
const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode))
return false;
- /* When -minterlink-mips16 is in effect, assume that non-locally-binding
- functions could be MIPS16 ones unless an attribute explicitly tells
+ /* When -minterlink-compressed is in effect, assume that
non-locally-binding
+ functions could be MIPS16 or microMIPS unless an attribute explicitly
tells
us otherwise. */
- if (TARGET_INTERLINK_MIPS16
+ if (TARGET_INTERLINK_COMPRESSED
decl
(DECL_EXTERNAL (decl) || !targetm.binds_local_p (decl))
- !mips_nomips16_decl_p (decl)
+ (compression_mode MASK_MICROMIPS) == 0
+ (compression_mode MASK_MIPS16) == 0
const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode))
return false;
should be something like:
/* Direct Js are only possible to functions that use the same ISA encoding.
There is no JX counterpart of JALX. */
if (decl
const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode)
mips_call_may_need_jalx_p (decl))
return false;
with:
/* Return true if a call to DECL may need to use JALX. */
static bool
mips_call_may_need_jalx_p (decl)
{
/* If the current translation unit would use a different mode for DECL,
assume that the call needs JALX. */
if (mips_get_compress_mode (decl) != TARGET_COMPRESSION)
return true;
/* mips_get_compress_mode is always accurate for locally-binding
functions in the current translation unit. */
if (!DECL_EXTERNAL (decl) targetm.binds_local_p (decl))
return false;
/* PLTs use the standard encoding, so calls that might go via PLTs
must use JALX. */
if (TARGET_COMPRESSED
TARGET_ABICALLS_PIC0
DECL_EXTERNAL (decl)
!targetm.binds_local_p (decl))
return true;
/* When -minterlink-compressed is in effect, assume that functions
could use a different encoding mode unless an attribute explicitly
tells us otherwise. */
if (TARGET_INTERLINK_COMPRESSED)
{
if (!TARGET_COMPRESSION mips_get_compress_off_flags (decl) == 0)
return true;
if (TARGET_COMPRESSION mips_get_compress_on_flags (decl) == 0)
return true;
}
return false;
}
The TARGET_ABICALLS_PIC0 case should deal with the gnu-user.h comment above.
How about instead of complicating this we simply add support for
microMIPS encoding in the PLT? I think I should be able to squeeze out
some time next week to dust off and retest the binutils patch I've had
pending far too long now. This way we won't have to maintain separate
cases where tail calls may or may not be made via the PLT.
Note that we need that support sooner or later anyway due to the prospect
of pure-microMIPS processors.
Maciej
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Hi Catherine,
Thanks for the update. Despite the length of this reply, there isn't
anything major.
Please don't change all NOMIPS16 to NOCOMPRESSION in the testsuite.
Most NOMIPSs are there because of the drastically reduced MIPS16
instruction set. I would have expected most of them to pass with
microMIPS.
Instead, please add NOCOMPRESSION as an alternative to NOMIPS16,
and only change NOMIPS16 to NOCOMPRESSION in tests that also fail for
microMIPS. I think that should be a separate patch and isn't really
a prerequisite for the core patch going in, although obviously it would
better if the testsuite results were clean. Please explain why each
NOMIPS16 to NOCOMPRESSION change is needed; even if it's obvious to you,
it probably won't be to me.
There need to be more test casees. We should at least test:
* LWM and SWM (see gcc.target/mips/save-restore-* for possible templates)
* MOVEP
* positive LWP and SWP tests (rather than just the volatile not used one)
* the new asm constraints
(Unlike the NOCOMPRESSION bits, these should be part of the core patch.)
As a general comment, I think it would be good to have:
/* The ISA compression flags that are currently in effect. */
#define TARGET_COMPRESSION (target_flags (MASK_MIPS16 | MASK_MICROMIPS))
I've used this in some of the comments below.
Moore, Catherine catherine_mo...@mentor.com writes:
Index: config/mips/linux-unwind.h
===
--- config/mips/linux-unwind.h(revision 195304)
+++ config/mips/linux-unwind.h(working copy)
@@ -52,6 +52,11 @@
_Unwind_Ptr new_cfa, reg_offset;
int i;
+ /* microMIPS frame; the kernel does not have microMIPS signal
+ frames. */
+ if ((_Unwind_Ptr) pc 3)
+return _URC_END_OF_STACK;
Should mention MIPS16 too. E.g.:
/* A MIPS16 or microMIPS frame. Signal frames always use the standard
ISA encoding. */
@@ -15991,18 +15995,33 @@ Generate MIPS16 code on alternating functions. Th
for regression testing of mixed MIPS16/non-MIPS16 code generation, and is
not intended for ordinary use in compiling user code.
+@item -minterlink-compressed
+@item -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.
I think I'm reediting a previous suggestion, sorry, but:
For example, code using the standard ISA encoding cannot jump directly
to MIPS16 or microMIPS code; it must either use a call or an indirect jump.
@option{-minterlink-compressed} therefore disables direct jumps unless GCC
knows that the target of the jump is not compressed.
+@item -minterlink-uncompressed
+@item -mno-interlink-uncompressed
+@opindex minterlink-compressed
+@opindex mno-interlink-compressed
+Require (do not require) that code using microMIPS instructions be
+link-compatible with the standard (uncompressed) MIPS ISA.
The distinction between this and -minterlink-compressed seems confusing
to me, and it wasn't handled consistently in the patch (see the sibcall
and gnu-user.h comments below). I think it would be better to fold the
functionality into -minterlink-compressed, with the documentation
changed to:
Require (do not require) that code using the standard (uncompressed) MIPS ISA
be link-compatible with MIPS16 and microMIPS code, and vice versa.
Index: config/mips/gnu-user.h
===
--- config/mips/gnu-user.h(revision 195351)
+++ config/mips/gnu-user.h(working copy)
@@ -137,3 +137,12 @@ extern const char *host_detect_local_cpu (int argc
#define ENDFILE_SPEC \
GNU_USER_TARGET_MATHFILE_SPEC \
GNU_USER_TARGET_ENDFILE_SPEC
+
+#undef SUBTARGET_OVERRIDE_OPTIONS
+#define SUBTARGET_OVERRIDE_OPTIONS \
+do {\
+ /* microMIPS PLT entries are non-microMIPS. */ \
+ TARGET_INTERLINK_COMPRESSED = 1; \
+} while (0)
+
+
Hmm, that sounds like a reason to set TARGET_INTERLINK_UNCOMPRESSED
rather than TARGET_INTERLINK_COMPRESSED. I.e. we need to avoid direct
branches from microMIPS code to standard PLTs.
But that means that microMIPS code can't even jump directly to functions
that have a micromips attribute when the call might go via a PLT.
TARGET_INTERLINK_(UN)COMPRESSED doesn't cover that case. I think instead
we need to handle it directly in mips_function_ok_for_sibcall,
keyed off TARGET_ABICALLS_PIC0. Specific
RE: FW: [PATCH] [MIPS] microMIPS gcc support
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 slots have
both been deferred. You had suggested a function named
mips_get_compression_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 some of the MIPS16-specific tests will fail. It looks like an
overhaul of mips.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 patch
is approved.
Thanks,
Catherine
-Original Message-
From: Richard Sandiford [mailto:rdsandif...@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
Moore, Catherine catherine_mo...@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
Re: FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine catherine_mo...@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
