This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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:rdsandiford@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_Moore@mentor.com> writes:
> >> -----Original Message-----
> >> From: Richard Sandiford [mailto:rdsandiford@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

Attachment: short-delay.cl
Description: short-delay.cl

Attachment: short-delay.patch
Description: short-delay.patch


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]