> -----Original Message----- > From: Richard Sandiford [mailto:rdsandif...@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_mo...@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
short-delay-03-21.cl
Description: short-delay-03-21.cl
short-delay-03-21.patch
Description: short-delay-03-21.patch
