FW: [PATCH] [MIPS] microMIPS gcc support

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

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.

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)
	(*mov<mode>_internal, *mov<mode>_mips16): Update accordingly.

> -----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.

Patch is attached.  WDYT?
I'm still testing, but results so far look really good.
Catherine

"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?  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.

> +(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)

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

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

"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.

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

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@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_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.
> 
> 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 constraints here would take ZS-ZZ.  The base microMIPS patch used ZC and ZD and ZR has been used.
That leaves ZA, ZB, and ZD to ZQ.
Catherine

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford [mailto:rdsandiford@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_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.
>> 
>> 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 constraints.  I'll fix that up.
> Adding the new microMIPS memory constraints here would take ZS-ZZ.  The
> base microMIPS patch used ZC and ZD and ZR has been used.
> That leaves ZA, ZB, and ZD to ZQ.

Sounds good, thanks.

Richard

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@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_Moore@mentor.com> writes:
> >> -----Original Message-----
> >> From: Richard Sandiford [mailto:rdsandiford@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_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.
> >>
> >> 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 constraints.  I'll fix that up.
> > Adding the new microMIPS memory constraints here would take ZS-ZZ.
> > The base microMIPS patch used ZC and ZD and ZR has been used.
> > That leaves ZA, ZB, and ZD to ZQ.
> 
> Sounds good, thanks.
> 
HI Richard,
Now done.  New patch and ChangeLog attached.  Thanks for reviewing.
Catherine

"Moore, Catherine" <Catherine_Moore@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 "Ue"s 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 "*add<mode>3"
> -  [(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 "sub<mode>3"
> -  [(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")))]
>    ""
> -  "<d>subu\t%0,%1,%2"
> +{ return "<d>subu\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 "*and<mode>3"
> -  [(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")
> +		 (match_operand:GPR 2 "and_operand" "Yb,Yh,Yw,Uean,K,Yx,Yw,!u,d")))]
>    "!TARGET_MIPS16 && and_operands_ok (<MODE>mode, operands[1], operands[2])"
>  {
>    int len;
> @@ -2897,20 +2924,23 @@
>        operands[1] = gen_lowpart (SImode, operands[1]);
>        return "lwu\t%0,%1";
>      case 3:
> +    case 4:
>        return "andi\t%0,%1,%x2";
> -    case 4:
> +    case 5:
>        len = low_bitmask_len (<MODE>mode, INTVAL (operands[2]));
>        operands[2] = GEN_INT (len);
>        return "<d>ext\t%0,%1,0,%2";
> -    case 5:
> +    case 6:
>        return "#";
> -    case 6:
> +    case 7:
> +    case 8:
>        return "and\t%0,%1,%2";
>      default:
>        gcc_unreachable ();
>      }
>  }
> -  [(set_attr "move_type" "load,load,load,andi,ext_ins,shift_shift,logical")
> +  [(set_attr "move_type" "load,load,load,andi,andi,ext_ins,shift_shift,logical,logical")
> +   (set_attr "compression" "*,*,*,micromips,*,*,*,micromips,*")
>     (set_attr "mode" "<MODE>")])

Looks good.

> @@ -4362,13 +4398,14 @@
>  ;; in FP registers (off by default, use -mdebugh to enable).
>  
>  (define_insn "*mov<mode>_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,d,d,ZS,ZU,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,ZT,ZS,m,ks,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D"))]

The d <- ZS alternative (the 7th) should be !ks <- ZS instead.

>    "!TARGET_MIPS16
>     && (register_operand (operands[0], <MODE>mode)
>         || reg_or_0_operand (operands[1], <MODE>mode))"
>    { return mips_output_move (operands[0], operands[1]); }
> -  [(set_attr "move_type" "move,const,const,load,store,mtc,fpload,mfc,fpstore,mfc,mtc,mtlo,mflo,mtc,fpload,mfc,fpstore")
> +  [(set_attr "move_type" "move,move,const,const,load,load,load,load,store,store,store,mtc,fpload,mfc,fpstore,mfc,mtc,mtlo,mflo,mtc,fpload,mfc,fpstore")

I think the 5th alternative (!u <- Udb7) should be "const" rather than "load".

> +/* Return true if X fits within a signed field of  BITS bits that is
> +   shifted left SHIFT bits before being used.  */

Too many spaces before "BITS".

> +
> +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);
> +}

Formatting: there should be one extra space of indentantion.

Richard

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:rdsandiford@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_Moore@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 "Ue"s 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 "*add<mode>3"
> > -  [(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 "sub<mode>3"
> > -  [(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")))]
> >    ""
> > -  "<d>subu\t%0,%1,%2"
> > +{ return "<d>subu\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 "*and<mode>3"
> > -  [(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")
> > +		 (match_operand:GPR 2 "and_operand"
> > +"Yb,Yh,Yw,Uean,K,Yx,Yw,!u,d")))]
> >    "!TARGET_MIPS16 && and_operands_ok (<MODE>mode, operands[1],
> operands[2])"
> >  {
> >    int len;
> > @@ -2897,20 +2924,23 @@
> >        operands[1] = gen_lowpart (SImode, operands[1]);
> >        return "lwu\t%0,%1";
> >      case 3:
> > +    case 4:
> >        return "andi\t%0,%1,%x2";
> > -    case 4:
> > +    case 5:
> >        len = low_bitmask_len (<MODE>mode, INTVAL (operands[2]));
> >        operands[2] = GEN_INT (len);
> >        return "<d>ext\t%0,%1,0,%2";
> > -    case 5:
> > +    case 6:
> >        return "#";
> > -    case 6:
> > +    case 7:
> > +    case 8:
> >        return "and\t%0,%1,%2";
> >      default:
> >        gcc_unreachable ();
> >      }
> >  }
> > -  [(set_attr "move_type"
> > "load,load,load,andi,ext_ins,shift_shift,logical")
> > +  [(set_attr "move_type"
> "load,load,load,andi,andi,ext_ins,shift_shift,logical,logical")
> > +   (set_attr "compression" "*,*,*,micromips,*,*,*,micromips,*")
> >     (set_attr "mode" "<MODE>")])
> 
> Looks good.
> 
> > @@ -4362,13 +4398,14 @@
> >  ;; in FP registers (off by default, use -mdebugh to enable).
> >
> >  (define_insn "*mov<mode>_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,d,d,ZS,ZU,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,ZT,ZS,m,ks,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B
> > +*C*D,*B*C*D"))]
> 
> The d <- ZS alternative (the 7th) should be !ks <- ZS instead.
> 
> >    "!TARGET_MIPS16
> >     && (register_operand (operands[0], <MODE>mode)
> >         || reg_or_0_operand (operands[1], <MODE>mode))"
> >    { return mips_output_move (operands[0], operands[1]); }
> > -  [(set_attr "move_type"
> > "move,const,const,load,store,mtc,fpload,mfc,fpstore,mfc,mtc,mtlo,mflo,
> > mtc,fpload,mfc,fpstore")
> > +  [(set_attr "move_type"
> > + "move,move,const,const,load,load,load,load,store,store,store,mtc,fpl
> > + oad,mfc,fpstore,mfc,mtc,mtlo,mflo,mtc,fpload,mfc,fpstore")
> 
> I think the 5th alternative (!u <- Udb7) should be "const" rather than "load".
> 
> > +/* Return true if X fits within a signed field of  BITS bits that is
> > +   shifted left SHIFT bits before being used.  */
> 
> Too many spaces before "BITS".
> 
> > +
> > +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); }
> 
> Formatting: there should be one extra space of indentantion.
> 
> Richard

"Moore, Catherine" <Catherine_Moore@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 "*add<mode>3"
> -  [(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 "sub<mode>3"
> -  [(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")))]
>    ""
> -  "<d>subu\t%0,%1,%2"
> +{ return "<d>subu\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 "*mov<mode>_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,d,d,ZS,ZU,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,ZT,ZS,m,ks,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B*C*D,*B*C*D"))]

The d <- ZS alternative (the 7th) should be !ks <- ZS instead.

>    "!TARGET_MIPS16
>     && (register_operand (operands[0], <MODE>mode)
>         || reg_or_0_operand (operands[1], <MODE>mode))"
>    { return mips_output_move (operands[0], operands[1]); }
> -  [(set_attr "move_type" "move,const,const,load,store,mtc,fpload,mfc,fpstore,mfc,mtc,mtlo,mflo,mtc,fpload,mfc,fpstore")
> +  [(set_attr "move_type" "move,move,const,const,load,load,load,load,store,store,store,mtc,fpload,mfc,fpstore,mfc,mtc,mtlo,mflo,mtc,fpload,mfc,fpstore")

I think the 5th alternative (!u <- Udb7) should be "const" rather than "load".

> +/* Return true if X fits within a signed field of  BITS bits that is
> +   shifted left SHIFT bits before being used.  */

Too many spaces before "BITS".

> +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);
> +}

Formatting: there should be one extra space of indentantion.

> +}
> +
> +
> +/* Return true if X is a legitimate address that conforms to the requirements
> +   for a microMIPS LWSP or SWSP insn.  */

Only one blank line rather than two.

Richard

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@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_Moore@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 "*add<mode>3"
> > -  [(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 "sub<mode>3"
> > -  [(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")))]
> >    ""
> > -  "<d>subu\t%0,%1,%2"
> > +{ return "<d>subu\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 "*mov<mode>_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,d,d,ZS,ZU,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,ZT,ZS,m,ks,!u,dJ,*d*J,*m,*f,*f,*z,*d,*J*d,*a,*d,*m,*B
> > +*C*D,*B*C*D"))]
> 
> The d <- ZS alternative (the 7th) should be !ks <- ZS instead.
Now fixed.
Plus the new constraints have been added to the pattern.

> 
> >    "!TARGET_MIPS16
> >     && (register_operand (operands[0], <MODE>mode)
> >         || reg_or_0_operand (operands[1], <MODE>mode))"
> >    { return mips_output_move (operands[0], operands[1]); }
> > -  [(set_attr "move_type"
> > "move,const,const,load,store,mtc,fpload,mfc,fpstore,mfc,mtc,mtlo,mflo,
> > mtc,fpload,mfc,fpstore")
> > +  [(set_attr "move_type"
> > + "move,move,const,const,load,load,load,load,store,store,store,mtc,fpl
> > + oad,mfc,fpstore,mfc,mtc,mtlo,mflo,mtc,fpload,mfc,fpstore")
> 
> I think the 5th alternative (!u <- Udb7) should be "const" rather than "load".
Now fixed.
> 
> > +/* Return true if X fits within a signed field of  BITS bits that is
> > +   shifted left SHIFT bits before being used.  */
> 
> Too many spaces before "BITS".
> 
> > +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); }
> 
> Formatting: there should be one extra space of indentantion.
> 
> > +}
> > +
> > +
> > +/* Return true if X is a legitimate address that conforms to the
> requirements
> > +   for a microMIPS LWSP or SWSP insn.  */
> 
> Only one blank line rather than two.
> 
> Richard

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_Moore@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 "sub<mode>3"
> -  [(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")))]
>    ""
> -  "<d>subu\t%0,%1,%2"
> +  "@
> +   <d>subu\t%0,%1,%2
> +   <d>subu\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 "*mov<mode>_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

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@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_Moore@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 "sub<mode>3"
> > -  [(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")))]
> >    ""
> > -  "<d>subu\t%0,%1,%2"
> > +  "@
> > +   <d>subu\t%0,%1,%2
> > +   <d>subu\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 "*mov<mode>_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

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> 2013-03-21  Catherine Moore  <clm@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.
> 	(*add<mode>3): New operands. Record compression.
> 	(sub<mode>3): Likewise.
> 	(one_cmpl<mode>2): Likewise.
> 	(*and<mode>3): Likewise.
> 	(*ior<mode>3): Likewise.
> 	(unnamed pattern for xor): Likewise.
> 	(*zero_extend<SHORT:mode><GPR:mode>2): Likewise.
> 	(*<optab><mode>3): Likewise.
> 	(*mov<mode>_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.add<mode>3"

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

On 03/23/2013 02:07 AM, Richard Sandiford wrote:
> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
>> 2013-03-21  Catherine Moore  <clm@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.
>> 	(*add<mode>3): New operands. Record compression.
>> 	(sub<mode>3): Likewise.
>> 	(one_cmpl<mode>2): Likewise.
>> 	(*and<mode>3): Likewise.
>> 	(*ior<mode>3): Likewise.
>> 	(unnamed pattern for xor): Likewise.
>> 	(*zero_extend<SHORT:mode><GPR:mode>2): Likewise.
>> 	(*<optab><mode>3): Likewise.
>> 	(*mov<mode>_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.add<mode>3"
>
> 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

I just tried to bootstrap on o32 Debian.  This system has binutils 2.20.1.

Here is a sample of the resulting failure when building the libjava 
target libs:
.
.
.
  /home/daney/gccsvn/build/./gcc/xgcc -B/home/daney/gccsvn/build/./gcc/ 
-B/usr/local/mips-unknown-linux-gnu/bin/ 
-B/usr/local/mips-unknown-linux-gnu/lib/ -isystem 
/usr/local/mips-unknown-linux-gnu/include -isystem 
/usr/local/mips-unknown-linux-gnu/sys-include -DHAVE_CONFIG_H -I. 
-I../../../../trunk/libjava/libltdl -g -O2 -minterlink-mips16 -c 
../../../../trunk/libjava/libltdl/ltdl.c  -fPIC -DPIC -o .libs/ltdl.o
/tmp/cckECtVQ.s: Assembler messages:
/tmp/cckECtVQ.s:12: Warning: Tried to set unrecognized symbol: nomicromips

/tmp/cckECtVQ.s:115: Warning: Tried to set unrecognized symbol: nomicromips

/tmp/cckECtVQ.s:161: Warning: Tried to set unrecognized symbol: nomicromips
.
.
.

There are literally thousands and thousands of these warnings.

David Daney

On Fri, 12 Apr 2013, David Daney wrote:

> I just tried to bootstrap on o32 Debian.  This system has binutils 2.20.1.
> 
> Here is a sample of the resulting failure when building the libjava target
> libs:
> .
> .
> .
>  /home/daney/gccsvn/build/./gcc/xgcc -B/home/daney/gccsvn/build/./gcc/
> -B/usr/local/mips-unknown-linux-gnu/bin/
> -B/usr/local/mips-unknown-linux-gnu/lib/ -isystem
> /usr/local/mips-unknown-linux-gnu/include -isystem
> /usr/local/mips-unknown-linux-gnu/sys-include -DHAVE_CONFIG_H -I.
> -I../../../../trunk/libjava/libltdl -g -O2 -minterlink-mips16 -c
> ../../../../trunk/libjava/libltdl/ltdl.c  -fPIC -DPIC -o .libs/ltdl.o
> /tmp/cckECtVQ.s: Assembler messages:
> /tmp/cckECtVQ.s:12: Warning: Tried to set unrecognized symbol: nomicromips
> 
> /tmp/cckECtVQ.s:115: Warning: Tried to set unrecognized symbol: nomicromips
> 
> /tmp/cckECtVQ.s:161: Warning: Tried to set unrecognized symbol: nomicromips
> .
> .
> .
> 
> There are literally thousands and thousands of these warnings.

 Thanks for the report, I guess GCC should:

1. Detect in its `configure' script if GAS supports the pseudo-op and 
   refrain from producing it if it does not (or actually perhaps it may 
   never produce it by default as GAS defaults to the nomicromips mode 
   anyway); we have precedents for that already.

2. Refuse the -mmicromips option and terminate if GAS does not support the 
   micromips mode; we have precedents for that too.

3. If support for pure-microMIPS processors is added in the future, then
   refuse to select that processor (with -march=) or being configured for 
   that processor by default (with a `configure' option) altogether unless 
   GAS supports the micromips mode.

 Thoughts?

  Maciej

> -----Original Message-----
> From: Maciej W. Rozycki [mailto:macro@codesourcery.com]
> Sent: Friday, April 12, 2013 1:03 PM
> To: David Daney
> Cc: Moore, Catherine; gcc-patches@gcc.gnu.org; Richard Sandiford
> Subject: Re: Many warnings in MIPS port (Was: [PATCH] [MIPS] microMIPS
> gcc support)
> 
> On Fri, 12 Apr 2013, David Daney wrote:
> 
> > I just tried to bootstrap on o32 Debian.  This system has binutils 2.20.1.
> >
> > Here is a sample of the resulting failure when building the libjava
> > target
> > libs:
> > .
> > .
> > .
> >  /home/daney/gccsvn/build/./gcc/xgcc -
> B/home/daney/gccsvn/build/./gcc/
> > -B/usr/local/mips-unknown-linux-gnu/bin/
> > -B/usr/local/mips-unknown-linux-gnu/lib/ -isystem
> > /usr/local/mips-unknown-linux-gnu/include -isystem
> > /usr/local/mips-unknown-linux-gnu/sys-include -DHAVE_CONFIG_H -I.
> > -I../../../../trunk/libjava/libltdl -g -O2 -minterlink-mips16 -c
> > ../../../../trunk/libjava/libltdl/ltdl.c  -fPIC -DPIC -o .libs/ltdl.o
> > /tmp/cckECtVQ.s: Assembler messages:
> > /tmp/cckECtVQ.s:12: Warning: Tried to set unrecognized symbol:
> > nomicromips
> >
> > /tmp/cckECtVQ.s:115: Warning: Tried to set unrecognized symbol:
> > nomicromips
> >
> > /tmp/cckECtVQ.s:161: Warning: Tried to set unrecognized symbol:
> > nomicromips .
> > .
> > .
> >
> > There are literally thousands and thousands of these warnings.
> 
>  Thanks for the report, I guess GCC should:
> 
> 1. Detect in its `configure' script if GAS supports the pseudo-op and
>    refrain from producing it if it does not (or actually perhaps it may
>    never produce it by default as GAS defaults to the nomicromips mode
>    anyway); we have precedents for that already.

Configure was modified as part of the micromips patch to detect support for the .set <no>micromips pseudo op.
Do you have a configure log?

>

On 04/12/2013 10:55 AM, Moore, Catherine wrote:
>
>
>> -----Original Message-----
>> From: Maciej W. Rozycki [mailto:macro@codesourcery.com]
>> Sent: Friday, April 12, 2013 1:03 PM
>> To: David Daney
>> Cc: Moore, Catherine; gcc-patches@gcc.gnu.org; Richard Sandiford
>> Subject: Re: Many warnings in MIPS port (Was: [PATCH] [MIPS] microMIPS
>> gcc support)
>>
>> On Fri, 12 Apr 2013, David Daney wrote:
>>
>>> I just tried to bootstrap on o32 Debian.  This system has binutils 2.20.1.
>>>
>>> Here is a sample of the resulting failure when building the libjava
>>> target
>>> libs:
>>> .
>>> .
>>> .
>>>   /home/daney/gccsvn/build/./gcc/xgcc -
>> B/home/daney/gccsvn/build/./gcc/
>>> -B/usr/local/mips-unknown-linux-gnu/bin/
>>> -B/usr/local/mips-unknown-linux-gnu/lib/ -isystem
>>> /usr/local/mips-unknown-linux-gnu/include -isystem
>>> /usr/local/mips-unknown-linux-gnu/sys-include -DHAVE_CONFIG_H -I.
>>> -I../../../../trunk/libjava/libltdl -g -O2 -minterlink-mips16 -c
>>> ../../../../trunk/libjava/libltdl/ltdl.c  -fPIC -DPIC -o .libs/ltdl.o
>>> /tmp/cckECtVQ.s: Assembler messages:
>>> /tmp/cckECtVQ.s:12: Warning: Tried to set unrecognized symbol:
>>> nomicromips
>>>
>>> /tmp/cckECtVQ.s:115: Warning: Tried to set unrecognized symbol:
>>> nomicromips
>>>
>>> /tmp/cckECtVQ.s:161: Warning: Tried to set unrecognized symbol:
>>> nomicromips .
>>> .
>>> .
>>>
>>> There are literally thousands and thousands of these warnings.
>>
>>   Thanks for the report, I guess GCC should:
>>
>> 1. Detect in its `configure' script if GAS supports the pseudo-op and
>>     refrain from producing it if it does not (or actually perhaps it may
>>     never produce it by default as GAS defaults to the nomicromips mode
>>     anyway); we have precedents for that already.
>
> Configure was modified as part of the micromips patch to detect support for the .set <no>micromips pseudo op.
> Do you have a configure log?

Here is the relevant fragment:
.
.
.
configure:25761: checking assembler for .micromips support
configure:25770: /usr/bin/as    -o conftest.o conftest.s >&5
conftest.s: Assembler messages:
conftest.s:1: Warning: Tried to set unrecognized symbol: micromips

configure:25773: $? = 0
configure:25784: result: yes
.
.
.

Since it is a warning, it succeeds.  I think you need to adjust the test 
so that it fails if there is a warning.








>
>>
>
>
>