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