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. > >> > >> > >> U<type><factor><bits> > >> > >> 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 "Ue"s, double digits for > other "U"s.) I think we should either: > > (a) use the same number of digits for all "U" constraints. That leaves > one character for the "Ue" type, which isn't as mnemonic, but is in > line with what we do elsewhere. > > (b) avoid digits in the "Ue" forms and just have an ad-hoc letter combination. > > Please keep "U" for constants though. The memory constraints should go > under "Z" instead (and therefore be only two letters long). The idea is that > the first letter of the constraint tells you what type it is. > > I don't think there's any need for the "Ue" constraints to have predicates of > the same name. We can go with longer, mnemonic, names instead. The idea > behind suggesting "sb4_operand", etc., was that (a) every character was > predictable and (b) I'm not sure the extra verbosity of (say) > "signed_byte_4_operand" would help. > But "addiur2_operand" would be good. > > > +(define_constraint "Udb07" > > + "@internal > > + A decremented unsigned constant of 7 bits." > > + (match_operand 0 "Udb07_operand" "")) > > Very minor nit, but these "" are redundant. > > > +(define_constraint "Ueim4" > > + "@internal > > + A microMIPS encoded ADDIUR2 immediate operand." > > + (match_operand 0 "Ueim4_operand" "")) > > Again minor, but the name doesn't really seem to match the description. > Is this constraint needed for things other than ADDIUR2? 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 "U"s in the predicate names. > > > +(define_attr "compression" "none,all,micromips,mips16" > > + (const_string "none")) > > Thinking about it a bit more, it would probably be better to leave the "all" > and > "mips16" values out until we need them. You probably missed it, but there is one use of the "all constraint in the patch. It's the first alternative in the *mov<mode>_internal pattern. I've removed the "mips16" value in the current patch. > > > +(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 "<d><insn>\t%0,%1,%2"; > > } > > [(set_attr "type" "shift") > > + (set_attr_alternative "compression" > > + [(if_then_else (ior (match_test "GET_CODE (SET_SRC (PATTERN > (insn))) == ASHIFT") > > + (match_test "GET_CODE (SET_SRC (PATTERN > (insn))) == LSHIFTRT")) > > + (const_string "micromips") > > + (const_string "none")) > > + (const_string "none")]) > > (set_attr "mode" "<MODE>")]) > > This would probably be better as: > > (define_code_attr shift_compression [(ashift "micromips") > (lshiftrt "micromips") > (ashiftrt "none")]) ... > (set_attr "compression" "micromips,<shift_compression>") > > which makes the distinction at compile time. > > The mips.md changes look good otherwise though. > > One idea (just as a nice-to-have) is that we could add an option that makes > the asm output routines add ".16" to mnemonics that we think are > 16 bits long, as a bit of extra sanity checking. That would be a separate > patch > though and is definitely not a requirement. > > > +/* Return true if X is a legitimate address that conforms to the > requirements > > + for any of the short microMIPS LOAD or STORE instructions except LWSP > > + or SWSP. */ > > + > > +bool > > +umips_address_p (rtx x, bool load, enum machine_mode mode) { > > + > > + struct mips_address_info addr; > > + bool ok = mips_classify_address (&addr, x, mode, false) > > + && addr.type == ADDRESS_REG > > + && M16_REG_P (REGNO (addr.reg)) > > + && CONST_INT_P (addr.offset); > > + > > + if (!ok) > > + return false; > > + > > + if (mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2) > > + || mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 1)) > > + return true; > > + > > + if (load && IN_RANGE (INTVAL (addr.offset), -1, 14)) > > + return true; > > + > > + if (!load && IN_RANGE (INTVAL (addr.offset), 0, 15)) > > + return true; > > + > > + return false; > > + > > +} > > No blank lines after "{" and before "}". > > More importantly, what cases are these range tests covering? > Both binutils and qemu seem to think that LW and SW need offsets that > exactly match: > > mips_unsigned_immediate_p (INTVAL (addr.offset), 4, 2) > 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. > i.e.: > > bool > umips_address_p (rtx x, enum machine_mode mode) { > struct mips_address_info addr; > if (mips_classify_address (&addr, x, mode, false) > && addr.type == ADDRESS_REG > && M16_REG_P (REGNO (addr.reg)) > && uw4_operand (addr.offset)) > return true; > } > > with no load vs. store check. > > > +/* Return true if X is a legitimate address that conforms to the > requirements > > + for a microMIPS LWSP or SWSP insn. */ > > + > > +bool > > +umips_lwsp_swsp_address_p (rtx x, enum machine_mode mode) { > > + struct mips_address_info addr; > > + > > + return (mips_classify_address (&addr, x, mode, false) > > + && addr.type == ADDRESS_REG > > + && REGNO (addr.reg) == STACK_POINTER_REGNUM > > + && CONST_INT_P (addr.offset) > > + && mips_unsigned_immediate_p (INTVAL (addr.offset), 5, 2)); > > I'd prefer: > > return (mips_classify_address (&addr, x, mode, false) > && addr.type == ADDRESS_REG > && REGNO (addr.reg) == STACK_POINTER_REGNUM > && uw5_operand (addr.offset)); > > > @@ -8010,9 +8075,17 @@ mips_print_operand_punctuation (FILE *file, int > > ch > > > > case '!': > > /* When final_sequence is 0, the delay slot will be a nop. We can > > - a 16-bit delay slot for microMIPS. */ > > + use a 16-bit delay slot for microMIPS. */ > > if (final_sequence == 0) > > putc ('s', file); > > + else > > + { > > + /* Use the compact version for microMIPS if the delay slot is a > > + compact microMIPS instruction. */ > > + rtx insn = XVECEXP (final_sequence, 0, 1); > > + if (get_attr_length (insn) == 2) > > + putc ('s', file); > > + } > > I think this is easier as: > > if (final_sequence == 0 > || get_attr_length (XVECEXP (final_sequence, 0, 1)) == 2) > putc ('s', file); > > Richard
short-delay.cl
Description: short-delay.cl
short-delay.patch
Description: short-delay.patch
