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:rdsandif...@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_mo...@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

Attachment: short-delay-03.20.patch
Description: short-delay-03.20.patch

Reply via email to