FW: [PATCH] [MIPS] microMIPS gcc support

[Richard Sandiford]

"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