Re: FW: [PATCH] [MIPS] microMIPS gcc support

[Richard Sandiford <rdsandiford at googlemail dot com>]

"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