This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Richard,

A new patch that incorporates your latest suggestions is attached.  I hope that we are converging on an implementation.

I have at least three follow-on patches to submit (short delay slot identification, jraddiusp, and testsuite cleanup). 

You had requested that the -mno-jals option be changed to -mno-interlink-uncompress.  I'd like to support -mno-jals (as an alias for -mno-interlink-uncompress) to accommodate folks who are already using that option.  If you don't have objections, I will submit that as a follow-on patch as well.

As usual, thanks for the thorough review.
Catherine

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Wednesday, January 23, 2013 3:05 PM
> To: Moore, Catherine
> Cc: gcc-patches@gcc.gnu.org; Rozycki, Maciej
> Subject: Re: FW: [PATCH] [MIPS] microMIPS gcc support
> 
> Hi Catherine,
> 
> Thanks for the update.  Despite the length of this reply, there isn't anything
> major.
> 
> Please don't change all NOMIPS16 to NOCOMPRESSION in the testsuite.
> Most NOMIPSs are there because of the drastically reduced MIPS16
> instruction set.  I would have expected most of them to pass with
> microMIPS.
> 
> Instead, please add NOCOMPRESSION as an alternative to NOMIPS16, and
> only change NOMIPS16 to NOCOMPRESSION in tests that also fail for
> microMIPS.  I think that should be a separate patch and isn't really a
> prerequisite for the core patch going in, although obviously it would better if
> the testsuite results were clean.  Please explain why each
> NOMIPS16 to NOCOMPRESSION change is needed; even if it's obvious to
> you, it probably won't be to me.
> 
> There need to be more test casees.  We should at least test:
> 
> * LWM and SWM (see gcc.target/mips/save-restore-* for possible
> templates)
> * MOVEP
> * positive LWP and SWP tests (rather than just the volatile "not used" one)
> * the new asm constraints
> 
> (Unlike the NOCOMPRESSION bits, these should be part of the core patch.)
> 
> 
> As a general comment, I think it would be good to have:
> 
> /* The ISA compression flags that are currently in effect.  */ #define
> TARGET_COMPRESSION (target_flags & (MASK_MIPS16 |
> MASK_MICROMIPS))
> 
> I've used this in some of the comments below.
> 
> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> > Index: config/mips/linux-unwind.h
> >
> ==========================================================
> =========
> > --- config/mips/linux-unwind.h	(revision 195304)
> > +++ config/mips/linux-unwind.h	(working copy)
> > @@ -52,6 +52,11 @@
> >    _Unwind_Ptr new_cfa, reg_offset;
> >    int i;
> >
> > +  /* microMIPS frame; the kernel does not have microMIPS signal
> > +     frames.  */
> > +  if ((_Unwind_Ptr) pc & 3)
> > +    return _URC_END_OF_STACK;
> 
> Should mention MIPS16 too.  E.g.:
> 
>   /* A MIPS16 or microMIPS frame.  Signal frames always use the standard
>      ISA encoding.  */
> 
> > @@ -15991,18 +15995,33 @@ Generate MIPS16 code on alternating
> > functions.  Th  for regression testing of mixed MIPS16/non-MIPS16 code
> > generation, and is  not intended for ordinary use in compiling user code.
> >
> > +@item -minterlink-compressed
> > +@item -mno-interlink-compressed
> > +@opindex minterlink-compressed
> > +@opindex mno-interlink-compressed
> > +Require (do not require) that code using the standard (uncompressed)
> > +MIPS ISA be link-compatible with MIPS16 and microMIPS code.
> > +
> > +For example, non-MIPS16 code cannot jump directly to MIPS16 code; it
> > +must either use a call or an indirect jump.  The same applies to
> > +non-microMIPS jumps to microMIPS code.
> > +@option{-minterlink-compressed} therefore disables direct jumps unless
> GCC knows that the target of the jump is not compressed.
> 
> I think I'm reediting a previous suggestion, sorry, but:
> 
>   For example, code using the standard ISA encoding cannot jump directly
>   to MIPS16 or microMIPS code; it must either use a call or an indirect jump.
>   @option{-minterlink-compressed} therefore disables direct jumps unless
> GCC
>   knows that the target of the jump is not compressed.
> 
> > +@item -minterlink-uncompressed
> > +@item -mno-interlink-uncompressed
> > +@opindex minterlink-compressed
> > +@opindex mno-interlink-compressed
> > +Require (do not require) that code using microMIPS instructions be
> > +link-compatible with the standard (uncompressed) MIPS ISA.
> 
> The distinction between this and -minterlink-compressed seems confusing to
> me, and it wasn't handled consistently in the patch (see the sibcall and gnu-
> user.h comments below).  I think it would be better to fold the functionality
> into -minterlink-compressed, with the documentation changed to:
> 
>   Require (do not require) that code using the standard (uncompressed)
> MIPS ISA
>   be link-compatible with MIPS16 and microMIPS code, and vice versa.
> 
> > Index: config/mips/gnu-user.h
> >
> ==========================================================
> =========
> > --- config/mips/gnu-user.h	(revision 195351)
> > +++ config/mips/gnu-user.h	(working copy)
> > @@ -137,3 +137,12 @@ extern const char *host_detect_local_cpu (int
> > argc  #define ENDFILE_SPEC \
> >    GNU_USER_TARGET_MATHFILE_SPEC " " \
> >    GNU_USER_TARGET_ENDFILE_SPEC
> > +
> > +#undef SUBTARGET_OVERRIDE_OPTIONS
> > +#define SUBTARGET_OVERRIDE_OPTIONS                              \
> > +do {                                                            \
> > +  /* microMIPS PLT entries are non-microMIPS.  */               \
> > +  TARGET_INTERLINK_COMPRESSED = 1;                              \
> > +} while (0)
> > +
> > +
> 
> Hmm, that sounds like a reason to set TARGET_INTERLINK_UNCOMPRESSED
> rather than TARGET_INTERLINK_COMPRESSED.  I.e. we need to avoid direct
> branches from microMIPS code to standard PLTs.
> 
> But that means that microMIPS code can't even jump directly to functions
> that have a micromips attribute when the call might go via a PLT.
> TARGET_INTERLINK_(UN)COMPRESSED doesn't cover that case.  I think
> instead we need to handle it directly in mips_function_ok_for_sibcall, keyed
> off TARGET_ABICALLS_PIC0.  Specific suggestion below.
> 
> > +(define_insn "*lwp"
> > +  [(parallel [(set (match_operand:SI 0 "d_operand")
> > +		   (match_operand:SI 1 "non_volatile_mem_operand"))
> > +	      (set (match_operand:SI 2 "d_operand")
> > +		   (match_operand:SI 3 "non_volatile_mem_operand"))])]
> > +
> > +  "TARGET_MICROMIPS
> > +   && umips_load_store_pair_p (true, operands)"
> > +  {
> > +    umips_output_load_store_pair (true, operands);
> > +    return "";
> > +  }
> 
> Very minor, sorry, but other MIPS patterns put multiline { ... } blocks in
> column 0.  Same for *swp, but *movep<MOVEP1:mode><MOVEP2:mode> is
> already OK.
> 
> > +  [(set_attr "type" "load")
> > +   (set_attr "mode" "SI")
> > +   (set_attr "can_delay" "no")])
> 
> Please add a comment saying why LWP can't go in a delay slot.
> Same for SWP and MOVEP.
> 
> > +(define_insn "mips_jraddiusp"
> > +  [(parallel [(return)
> > +              (use (reg:SI 31))
> > +	      (set (reg:SI 29)
> > +		   (plus:SI (reg:SI 29)
> > +			    (match_operand 0 "const_int_operand")))])]
> > +  "TARGET_MICROMIPS"
> > +  "jraddiusp\t%0"
> > +  [(set_attr "type" "trap")
> > +   (set_attr "mode" "SI")
> > +   (set_attr "can_delay" "no")])
> 
> Is this pattern used?  If not, please leave it out for now.
> If it is used, please add a testcase.
> 

> > +; For movep
> > +(define_peephole2
> > +  [(set (match_operand:MOVEP1 0 "register_operand" "")
> > +        (match_operand:MOVEP1 1 "movep_operand" ""))
> > +   (set (match_operand:MOVEP2 2 "register_operand" "")
> > +        (match_operand:MOVEP2 3 "movep_operand" ""))]
> > +  "TARGET_MICROMIPS
> > +   && umips_movep_target_p (operands[0], operands[2])"
> > +  [(parallel [(set (match_dup 0) (match_dup 1))
> > +              (set (match_dup 2) (match_dup 3))])]
> > +)
> 
> Probably not worth having movep_operand, because all the checking is done
> by umips_movep_target_p.  reg_or_0_operand should be OK.      

The source and target register criteria are not identical.  I've now renamed the movep_operand predicate to movep_src_operand. 
> 
> > Index: config/mips/constraints.md
> >
> ==========================================================
> =========
> > --- config/mips/constraints.md	(revision 195351)
> > +++ config/mips/constraints.md	(working copy)
> > @@ -232,6 +232,26 @@
> >     "@internal"
> >     (match_operand 0 "low_bitmask_operand"))
> >
> > +(define_memory_constraint "ZC"
> > +  "When compiling microMIPS code, this constraing matches a memory
> operand
> > +   whose address is formed from a base register and a 12-bit offset.  These
> > +   operands can be used for microMIPS instructions such as @code{ll} and
> > +   @code{sc}.  When not compiling for microMIPS code, @code{ZC} is
> > +   equivalent to @code{R}."
> > +  (and (match_code "mem")
> > +       (ior (and (match_test "TARGET_MICROMIPS")
> > +		 (match_test "umips_12bit_offset_address_p (XEXP (op, 0),
> mode)"))
> > +	    (match_test "mips_address_insns (XEXP (op, 0), mode, false)"))))
> 
> The last line should be:
> 
>    (match_test "mips_address_insns (XEXP (op, 0), mode, false) == 1")
> 
> to match "R".  The documentation makes it sound like an if-then-else, but
> the implementation isn't.  Should it be:
> 
>   (and (match_code "mem")
>        (if_then_else
> 	 (match_test "TARGET_MICROMIPS")
> 	 (match_test "umips_12bit_offset_address_p (XEXP (op, 0),
> mode)"))
> 	 (match_test "mips_address_insns (XEXP (op, 0), mode, false) ==
> 1"))))
> 
> instead?
> 
> > +(define_address_constraint "ZD"
> > +  "When compiling microMIPS code, this constraint matches an address
> operand
> > +   that is formed from a base register and a 12-bit offset.  These operands
> > +   can be used for microMIPS instructions such as @code{prefetch}.  When
> > +   not compiling for microMIPS code, @code{YC} is equivalent to
> @code{p}."
> > +   (ior (and (match_test "TARGET_MICROMIPS")
> > +	     (match_test "umips_12bit_offset_address_p (op, mode)"))
> > +	(match_test "mips_address_insns (op, mode, false)")))
> 
> Same comments here.
> 
> > +;; Return 1 if the operand is in non-volatile memory.  Note that
> > +during the ;; RTL generation phase, memory_operand does not return
> > +TRUE for volatile ;; memory references.  So this function allows us
> > +to recognize volatile ;; references where it's safe.
> > +(define_predicate "non_volatile_mem_operand"
> > +  (and (match_operand 0 "memory_operand")
> > +       (not (match_test "MEM_VOLATILE_P (op)"))))
> 
> I don't understand the comment.  We're trying to avoid all volatile
> references.
> 
> > +minterlink-uncompressed
> > +Target Report Var(TARGET_INTERLINK_UNCOMPRESSED) Init(1)
> Generated
> > +code that is link-compatible with the standard (uncompressed) MIPS
> > +ISA.
> 
> Do multi-line documentation strings work?  Please check the --target-help
> output to make sure.
> 
Yes, this does work.

> > Index: config/mips/mips.c
> >
> ==========================================================
> =========
> > --- config/mips/mips.c	(revision 195351)
> > +++ config/mips/mips.c	(working copy)
> > @@ -77,6 +77,9 @@ along with GCC; see the file COPYING3.  If not see
> >     preserve the maximum stack alignment.  We therefore use a value
> >     of 0x7ff0 in this case.
> >
> > +   microMIPS LWM and SWM support 12-bit offsets (from -0x800 to 0x7ff),
> > +   so we use a maximum of 0x7f0 for TARGET_MICROMIPS.
> > +
> >     MIPS16e SAVE and RESTORE instructions can adjust the stack pointer by
> >     up to 0x7f8 bytes and can usually save or restore all the registers
> >     that we need to save or restore.  (Note that we can only use these
> > @@ -87,7 +90,8 @@ along with GCC; see the file COPYING3.  If not see
> >     to save and restore registers, and to allocate and deallocate the top
> >     part of the frame.  */
> >  #define MIPS_MAX_FIRST_STACK_STEP
> 	\
> > -  (!TARGET_MIPS16 ? 0x7ff0						\
> > +  ((!TARGET_MIPS16 && !TARGET_MICROMIPS) ? 0x7ff0
> 	\
> > +   : TARGET_MICROMIPS ? 0x7f0
> 	\
> >     : GENERATE_MIPS16E_SAVE_RESTORE ? 0x7f8
> 	\
> >     : TARGET_64BIT ? 0x100 : 0x400)
> 
> I'd prefer:
> 
>   (TARGET_MICROMIPS ? 0x7f0						\
>    : !TARGET_MIPS16 ? 0x7ff0						\
>    : GENERATE_MIPS16E_SAVE_RESTORE ? 0x7f8
> 	\
>    : TARGET_64BIT ? 0x100 : 0x400)
> 
> The comment doesn't really explain the reason for 0x7f0 rather than 0x7f8.
> Is this in anticipation of future n32 and n64 support, where the stack must be
> 16-byte aligned?  If so, that's OK, but worth mentioning.
> 
> > @@ -1167,10 +1174,11 @@ mflip_mips16_use_mips16_p (tree decl)
> >    const char *name;
> >    hashval_t hash;
> >    void **slot;
> > +  bool base_is_mips16 = mips_base_compression_flags & MASK_MIPS16;
> 
> Please make this:
> 
>   bool base_is_mips16 = (mips_base_compression_flags & MASK_MIPS16) !=
> 0;
> 
> > @@ -1257,12 +1252,57 @@ mips_use_debug_exception_return_p (tree
> type)
> >  			   TYPE_ATTRIBUTES (type)) != NULL;  }
> >
> > -/* Return true if function DECL is a MIPS16 function.  Return the ambient
> > -   setting if DECL is null.  */
> > +/* Return the set of compression modes that are explicitly required
> > +   by DECL.  */
> >
> > -static bool
> > -mips_use_mips16_mode_p (tree decl)
> > +static unsigned int
> > +mips_get_compress_on_flags (tree decl)
> >  {
> > +  unsigned int flags = 0;
> > +
> > +  if (!decl || !DECL_P (decl))
> > +    return flags;
> 
> Is this check necessary?  Same for mips_get_compress_off_flags.
> 
> > @@ -1297,29 +1337,47 @@ static void
> >  mips_insert_attributes (tree decl, tree *attributes)  {
> >    const char *name;
> > -  bool mips16_p, nomips16_p;
> > +  unsigned int compression_p, nocompression_p;
> 
> No _p, since these aren't booleans now.  _flags would be better.
> 
> >    /* Check for "mips16" and "nomips16" attributes.  */
> > -  mips16_p = lookup_attribute ("mips16", *attributes) != NULL;
> > -  nomips16_p = lookup_attribute ("nomips16", *attributes) != NULL;
> > +  compression_p = mips_get_compress_on_flags (decl);
> nocompression_p
> > + = mips_get_compress_off_flags (decl);
> > +
> >    if (TREE_CODE (decl) != FUNCTION_DECL)
> >      {
> > -      if (mips16_p)
> > +      if (compression_p & MASK_MIPS16)
> >  	error ("%qs attribute only applies to functions", "mips16");
> > -      if (nomips16_p)
> > +      if (nocompression_p & MASK_MIPS16)
> >  	error ("%qs attribute only applies to functions", "nomips16");
> > +
> > +      if (nocompression_p & MASK_MICROMIPS
> > +	  && nocompression_p & MASK_MIPS16)
> > +	error ("%qs attribute only applies to functions", "nocompression");
> 
> This case needs to come first, otherwise nomips16 will be reported instead.
> Although...
> 
> > +      else
> > +	{
> > +	  if (compression_p & MASK_MICROMIPS)
> > +	    error ("%qs attribute only applies to functions", "micromips");
> > +	  if (nocompression_p & MASK_MICROMIPS)
> > +	    error ("%qs attribute only applies to functions", "nomicromips");
> > +	}
> 
> ...I think it would be easier to have:
> 
> /* Return the attribute name associated with MASK_MIPS16 and
> MASK_MICROMIPS
>    flags FLAGS.  */
> 
> static const char *
> mips_get_compress_on_name (unsigned int flags) {
>   if (flags == MASK_MIPS16)
>     return "mips16";
>   return "micromips";
> }
> 
> /* Return the attribute that forbids MASK_MIPS16 and MASK_MICROMIPS
>    flags FLAGS.  */
> 
> static const char *
> mips_get_compress_off_name (unsigned int flags) {
>   if (flags == MASK_MIPS16)
>     return "nomips16";
>   if (flags == MASK_MICROMIPS)
>     return "nomicromips";
>   return "nocompression";
> }
> 
> Then:
> 
>       if (nocompression_flags)
>         error ("%qs attribute only applies to functions",
>                mips_get_compress_off_name (nocompression_flags));
>       if (compression_flags)
>         error ("%qs attribute only applies to functions",
>                mips_get_compress_on_name (compression_flags));
> 
> >      }
> >    else
> >      {
> > -      mips16_p |= mips_mips16_decl_p (decl);
> > -      nomips16_p |= mips_nomips16_decl_p (decl);
> > -      if (mips16_p || nomips16_p)
> > +      if (compression_p & MASK_MIPS16
> > +           && nocompression_p & (MASK_MIPS16 | MASK_MICROMIPS))
> >  	{
> > +	  /* DECL cannot be simultaneously "mips16" and "nocompression".
> */
> > +	  error ("%qE cannot have both %<mips16%> and "
> > +		 "%<nocompression%> attributes",
> > +		 DECL_NAME (decl));
> > +	}
> > +      else if (compression_p & MASK_MIPS16
> > +		&& nocompression_p & MASK_MIPS16)
> > +	{
> >  	  /* DECL cannot be simultaneously "mips16" and "nomips16".  */
> > -	  if (mips16_p && nomips16_p)
> > -	    error ("%qE cannot have both %<mips16%> and "
> > -		   "%<nomips16%> attributes",
> > -		   DECL_NAME (decl));
> > +	  error ("%qE cannot have both %<mips16%> and "
> > +		 "%<nomips16%> attributes",
> > +		 DECL_NAME (decl));
> >  	}
> >        else if (TARGET_FLIP_MIPS16 && !DECL_ARTIFICIAL (decl))
> >  	{
> > @@ -1329,6 +1387,39 @@ mips_insert_attributes (tree decl, tree
> *attribute
> >  	  name = mflip_mips16_use_mips16_p (decl) ? "mips16" :
> "nomips16";
> >  	  *attributes = tree_cons (get_identifier (name), NULL, *attributes);
> >  	}
> > +
> > +      if (compression_p & MASK_MICROMIPS
> > +           && nocompression_p & (MASK_MIPS16 | MASK_MICROMIPS))
> > +	{
> > +	  /* DECL cannot be simultaneously "micromips" and
> "nocompression".  */
> > +	  error ("%qE cannot have both %<micromips%> and "
> > +		 "%<nocompression%> attributes",
> > +		 DECL_NAME (decl));
> > +	}
> > +      else if (compression_p & MASK_MICROMIPS
> > +		&& nocompression_p & MASK_MICROMIPS)
> > +	{
> > +	  /* DECL cannot be simultaneously "micromips" and "nomicromips".
> */
> > +	  error ("%qs cannot have both %<micromips%> and "
> > +		 "%<nomicromips%> attributes",
> > +		 IDENTIFIER_POINTER (DECL_NAME (decl)));
> > +	}
> 
> All the above then becomes:
> 
>       if (compression_flags && nocompression_flags)
> 	error ("%qE cannot have both %qs and %qs attributes",
> 	       DECL_NAME (decl), mips_get_compress_on_name
> (compression_flags),
> 	       mips_get_compress_off_name (nocompression_flags));
> 
> > +
> > +      if (compression_p & MASK_MIPS16
> > +          && compression_p & MASK_MICROMIPS)
> > +	error ("%qs cannot have both %<mips16%> and %<micromips%>
> attributes",
> > +	       IDENTIFIER_POINTER (DECL_NAME (decl)));
> 
> %qE and no IDENTIFIER_POINTER.  Please use the %qs form here too, with
> "mips16" and "micromips" as error() arguments, so that one translation
> covers both this case and the previous one.
> 
> > +      /* If DECL is "nocompression" set the "nomips16" and
> > +	 "nomicromips" attributes.  */
> > +      if (nocompression_p & MASK_MIPS16
> > +          && nocompression_p & MASK_MICROMIPS)
> > +	{
> > +	  name = "nomips16";
> > +	  *attributes = tree_cons (get_identifier (name), NULL, *attributes);
> > +	  name = "nomicromips";
> > +	  *attributes = tree_cons (get_identifier (name), NULL, *attributes);
> > +	}
> 
> Hmm, why is this necessary?  Anything that cares should be using
> mips_get_compress_off_flags.
> 
> > @@ -1338,12 +1429,9 @@ static tree
> >  mips_merge_decl_attributes (tree olddecl, tree newdecl)  {
> >    /* The decls' "mips16" and "nomips16" attributes must match
> > exactly.  */
> > -  if (mips_mips16_decl_p (olddecl) != mips_mips16_decl_p (newdecl))
> > +  if (mips_get_compress_mode (olddecl) != mips_get_compress_mode
> > + (newdecl))
> >      error ("%qE redeclared with conflicting %qs attributes",
> > -	   DECL_NAME (newdecl), "mips16");
> > -  if (mips_nomips16_decl_p (olddecl) != mips_nomips16_decl_p
> (newdecl))
> > -    error ("%qE redeclared with conflicting %qs attributes",
> > -	   DECL_NAME (newdecl), "nomips16");
> > +	   DECL_NAME (newdecl), "compression");
> 
> This should be:
> 
>   unsigned int diff;
> 
>   diff = (mips_get_compress_on_flags (olddecl)
>           ^ mips_get_compress_on_flags (newdecl));
>   if (diff)
>     error ("%qE redeclared with conflicting %qs attributes",
>            DECL_NAME (newdecl), mips_get_compress_on_name (diff));
> 
>   diff = (mips_get_compress_off_flags (olddecl)
>           ^ mips_get_compress_off_flags (newdecl));
>   if (diff)
>     error ("%qE redeclared with conflicting %qs attributes",
>            DECL_NAME (newdecl), mips_get_compress_off_name (diff));
> 
> > @@ -2300,6 +2388,20 @@ mips_address_insns (rtx x, enum
> machine_mode mode,
> >    return 0;
> >  }
> >
> > +/* Return true if X is a legitimate address with a 12-bit offset.
> > +   MODE is the mode of the value being accessed.  */
> > +
> > +bool
> > +umips_12bit_offset_address_p (rtx x, enum machine_mode mode) {
> > +  struct mips_address_info addr;
> > +
> > +  return (mips_classify_address(&addr, x, mode, false)
> 
> Space before '('
> 
> > +	  && addr.type == ADDRESS_REG
> > +	  && CONST_INT_P (addr.offset)
> > +	  && UMIPS_12BIT_OFFSET_P (addr.offset));
> 
> Isn't there a missing INTVAL here?  Please check the patch to make sure
> there are no compiler warnings.
> 
> > @@ -6908,6 +7017,8 @@ mips_split_call (rtx insn, rtx call_pattern)
> > static bool  mips_function_ok_for_sibcall (tree decl, tree exp
> > ATTRIBUTE_UNUSED)  {
> > +  unsigned int compression_mode = mips_get_compress_mode (decl);
> > +
> 
> Might as well move this down to where it's needed, especially now that
> we're C++.
> 
> >    if (!TARGET_SIBCALLS)
> >      return false;
> >
> > @@ -6916,22 +7027,45 @@ mips_function_ok_for_sibcall (tree decl, tree
> exp
> >    if (mips_interrupt_type_p (TREE_TYPE (current_function_decl)))
> >      return false;
> >
> > +  if (TARGET_MICROMIPS)
> > +    {
> > +      /* We can't do a sibcall if the called function is a MIPS32
> > + function.  */
> 
> s/is a MIPS32 function/isn't microMIPS/, to avoid confusion with the MIPS32
> ISA.
> 
> > +      if (decl
> > +	  && (compression_mode & MASK_MICROMIPS) == 0
> > +	  && const_call_insn_operand (XEXP (DECL_RTL (decl), 0),
> VOIDmode))
> > +	return false;
> > +
> > +      /* When -minterlink-compressed is in effect, assume that non-locally-
> binding
> > +	 functions could be MIPS32 ones unless an attribute explicitly tells
> > +	 us otherwise.  */
> > +      if (TARGET_INTERLINK_COMPRESSED
> > +	  && decl
> > +	  && (DECL_EXTERNAL (decl) || !targetm.binds_local_p (decl))
> > +	  && (compression_mode & MASK_MICROMIPS) == 0
> > +	  && const_call_insn_operand (XEXP (DECL_RTL (decl), 0),
> VOIDmode))
> > +	return false;
> 
> We're dealing here with branches from microMIPS to non-microMIPS, so
> shouldn't this be TARGET_INTERLINK_UNCOMPRESSED?
> 
> We need to use mips_get_compress_on_flags rather than
> mips_get_compress_mode for the second condition, because we can only
> assume that external functions are micromips if they have been explicitly
> tagged.
> 
> > +
> > +      /* Otherwise OK.  */
> > +      return true;
> > +    }
> > +
> 
> There's too much cut-&-paste of the MIPS16 code here, so that it becomes
> difficult to see what the logic actually is.  With the suggested change to fold -
> minterlink-nocompressed into -minterlink-compressed, the block above and:
> 
> >    /* We can't do a sibcall if the called function is a MIPS16 function
> >       because there is no direct "jx" instruction equivalent to "jalx" to
> >       switch the ISA mode.  We only care about cases where the sibling
> >       and normal calls would both be direct.  */
> >    if (decl
> > -      && mips_use_mips16_mode_p (decl)
> > +      && ((compression_mode & (MASK_MIPS16 | MASK_MICROMIPS)) !=
> 0)
> >        && const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode))
> >      return false;
> >
> > -  /* When -minterlink-mips16 is in effect, assume that non-locally-binding
> > -     functions could be MIPS16 ones unless an attribute explicitly tells
> > +  /* When -minterlink-compressed is in effect, assume that non-locally-
> binding
> > +     functions could be MIPS16 or microMIPS unless an attribute
> > + explicitly tells
> >       us otherwise.  */
> > -  if (TARGET_INTERLINK_MIPS16
> > +  if (TARGET_INTERLINK_COMPRESSED
> >        && decl
> >        && (DECL_EXTERNAL (decl) || !targetm.binds_local_p (decl))
> > -      && !mips_nomips16_decl_p (decl)
> > +      && (compression_mode & MASK_MICROMIPS) == 0
> > +      && (compression_mode & MASK_MIPS16) == 0
> >        && const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode))
> >      return false;
> 
> should be something like:
> 
>   /* Direct Js are only possible to functions that use the same ISA encoding.
>      There is no JX counterpart of JALX.  */
>   if (decl
>       && const_call_insn_operand (XEXP (DECL_RTL (decl), 0), VOIDmode)
>       && mips_call_may_need_jalx_p (decl))
>     return false;
> 
> with:
> 
> /* Return true if a call to DECL may need to use JALX.  */
> 
> static bool
> mips_call_may_need_jalx_p (decl)
> {
>   /* If the current translation unit would use a different mode for DECL,
>      assume that the call needs JALX.  */
>   if (mips_get_compress_mode (decl) != TARGET_COMPRESSION)
>     return true;
> 
>   /* mips_get_compress_mode is always accurate for locally-binding
>      functions in the current translation unit.  */
>   if (!DECL_EXTERNAL (decl) && targetm.binds_local_p (decl))
>     return false;
> 
>   /* PLTs use the standard encoding, so calls that might go via PLTs
>      must use JALX.  */
>   if (TARGET_COMPRESSED
>       && TARGET_ABICALLS_PIC0
>       && DECL_EXTERNAL (decl)
>       && !targetm.binds_local_p (decl))
>     return true;
> 
>   /* When -minterlink-compressed is in effect, assume that functions
>      could use a different encoding mode unless an attribute explicitly
>      tells us otherwise.  */
>   if (TARGET_INTERLINK_COMPRESSED)
>     {
>       if (!TARGET_COMPRESSION && mips_get_compress_off_flags (decl) ==
> 0)
>         return true;
>       if (TARGET_COMPRESSION && mips_get_compress_on_flags (decl) == 0)
>         return true;
>     }
> 
>   return false;
> }
> 
> The TARGET_ABICALLS_PIC0 case should deal with the gnu-user.h comment
> above.
> 
> > @@ -7881,6 +8018,20 @@ mips_print_operand_punctuation (FILE *file, int
> ch
> >        fputs (reg_names[STACK_POINTER_REGNUM], file);
> >        break;
> >
> > +    case ':':
> > +      /* When reorder or noreorder with final_squence 0, the delay slot will
> > +	 be a nop, so we just use the compact version for microMIPS.  */
> > +      if (final_sequence == 0)
> > +	putc ('c', file);
> > +      break;
> > +
> > +    case '!':
> > +      /* When reorder or noreorder with final_squence 0, the delay slot will
> > +	 be a nop, so we just use the compact version for microMIPS.  */
> > +      if (final_sequence == 0)
> > +	putc ('s', file);
> > +      break;
> > +
> 
> Pasto: the second "the compact version" should be "16-bit delay slots".
> 
> > @@ -10198,7 +10349,7 @@ typedef void (*mips_save_restore_fn) (rtx,
> rtx);
> >     stack pointer.  */
> >
> >  static void
> > -mips_save_restore_reg (enum machine_mode mode, int regno,
> > +mips_save_restore_single_reg (enum machine_mode mode, int regno,
> >  		       HOST_WIDE_INT offset, mips_save_restore_fn fn)  {
> >    rtx mem;
> 
> Please reindent the second line, although I'd prefer not to have this change;
> more below.
> 
> > @@ -10208,6 +10359,23 @@ static void
> >    fn (gen_rtx_REG (mode, regno), mem);  }
> >
> > +static void
> > +mips_save_restore_registers (int start_reg, int end_reg, HOST_WIDE_INT
> *offset,
> > +			     mips_save_restore_fn fn, HOST_WIDE_INT
> sp_offset) {
> > +  int regno;
> > +
> > +  for (regno = start_reg; regno > end_reg; regno--)
> > +    if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> > +      {
> > +        /* Record the ra offset for use by mips_function_profiler.  */
> > +        if (regno == RETURN_ADDR_REGNUM)
> > +          cfun->machine->frame.ra_fp_offset = *offset + sp_offset;
> > +	mips_save_restore_single_reg (word_mode, regno, *offset, fn);
> > +	*offset -= UNITS_PER_WORD;
> > +      }
> > +}
> > +
> 
> Function lacks a comment, although I'm not sure it's needed; see below.
> 
> > +static const unsigned int type[19] = { 0x00010000, 0x00030000, 0x00070000,
> > +				       0x000f0000, 0x001f0000, 0x003f0000,
> > +				       0x007f0000, 0x00ff0000, 0x40ff0000,
> > +				       0x80000000, 0x80010000, 0x80030000,
> > +				       0x80070000, 0x800f0000, 0x801f0000,
> > +				       0x803f0000, 0x807f0000, 0x80ff0000,
> > +				       0xc0ff0000 };
> > +
> > +static const unsigned int encode[19] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 16, 17,
> > +					 18, 19, 20, 21, 22, 23, 24, 25};
> 
> Please give these longer names, since they have file scope.  E.g.:
> 
> /* The masks of GPRs that can be saved by a microMIPS SWM and restored
>    by an LWM.  */
> static const unsigned int umips_swm_mask[19] = {
>   0x00010000, 0x00030000, 0x00070000, 0x000f0000,
>   0x001f0000, 0x003f0000, 0x007f0000, 0x00ff0000, 0x40ff0000,
>   0x80000000, 0x80010000, 0x80030000, 0x80070000, 0x800f0000,
>   0x801f0000, 0x803f0000, 0x807f0000, 0x80ff0000, 0xc0ff0000 };
> 
> /* umips_swm_encoding[I] is the instruction encoding used to represent
> mask
>    umips_swm_mask[I].  */
> static const unsigned int umips_swm_encoding[19] = {
>   1, 2, 3, 4,
>   5, 6, 7, 8, 9,
>   16, 17, 18, 19, 20,
>   21, 22, 23, 24, 25
> };
> 
> > +/* Build microMIPS save or restore.  FN is save or restore function.
> > +   OFFSET is the current stack offset.
> > +   Return true if we succeed creating save or restore.  */
> > +
> > +static bool
> > +umips_build_save_restore (mips_save_restore_fn fn,
> > +			  HOST_WIDE_INT offset, HOST_WIDE_INT sp_offset)
> 
> The comment doesn't say what SP_OFFSET is, although with the comments
> below I think the prototype should be:
> 
> /* Try to use a microMIPS LWM or SWM instruction to save or restore
>    as many GPRs in *MASK as possible.  *OFFSET is the offset from the
>    stack pointer of the topmost save slot.
> 
>    Remove from *MASK all registers that were handled using LWM and SWM.
>    Update *OFFSET so that it points to the first unused save slot.  */
> 
> static void
> umips_build_save_restore (mips_save_restore_fn fn, unsigned *mask
> 			  HOST_WIDE_INT *offset)
> 
> > +{
> > +  int i, num_of_reg;
> > +  unsigned int j;
> > +  rtx pattern, set, reg, mem;
> > +  HOST_WIDE_INT this_offset;
> > +  rtx this_base;
> > +
> > +  /* LWM/SWM can only support offsets from -2048 to 2047.  */  if
> > + (!UMIPS_12BIT_OFFSET_P (offset))
> > +    return false;
> 
> At this point OFFSET is still the offset of the top of the save area, whereas the
> instruction operand is the address of the first saved register.
> I think you need to check this later, after:
> 
>   offset -= (UNITS_PER_WORD * (num_of_reg - 1));
> 
> > +  if (i < 8 && (cfun->machine->frame.mask & 0xff000000))
> > +    mips_save_restore_registers (GP_REG_LAST, GP_REG_LAST - 8,
> > +				 &offset, fn, sp_offset);
> 
> Any reason for handling this differently from the MIPS16 code?  I prefer
> passing back a mask of the unhandled registers, like it does.  It would make
> this function a bit simpler and would also allow the function to be used in
> future with masks that don't exactly match.  E.g. if we need to save and
> restore registers $16-$20 + $23, we could use LWM for $16-$20 and LW for
> $23.  The prototype suggested above was for this.
> 
> > +  /* Adjust offset for output.  */
> > +  num_of_reg = (encode[i] & 0xf) + (encode[i] >> 4);  offset -=
> > + (UNITS_PER_WORD * (num_of_reg - 1));
> 
> Very minor, sorry, but s/num_of_reg/nregs/;
> 
> > +  /* Create the final PARALLEL.  */
> > +  pattern = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (num_of_reg));
> > +
> > +  this_base = stack_pointer_rtx;
> > +  this_offset = offset;
> > +
> > +  /* For $16-$23, $30.  */
> > +  for (j = 0; j < (encode[i] & 0xf); j++)
> > +    {
> > +      unsigned int regno;
> > +      long long offset = this_offset + j * UNITS_PER_WORD;
> 
> s/long long/HOST_WIDE_INT/.
> 
> > +  pattern = emit_insn (pattern);
> > +  if (fn == mips_save_reg)
> > +    RTX_FRAME_RELATED_P(pattern) = 1;
> 
> Missing space before "(".
> 
> > @@ -10253,16 +10555,13 @@ mips_for_each_saved_gpr_and_fpr
> (HOST_WIDE_INT sp_
> >       need a nop in the epilogue if at least one register is reloaded in
> >       addition to return address.  */
> >    offset = cfun->machine->frame.gp_sp_offset - sp_offset;
> > -  for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
> > -    if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> > -      {
> > -	/* Record the ra offset for use by mips_function_profiler.  */
> > -	if (regno == RETURN_ADDR_REGNUM)
> > -	  cfun->machine->frame.ra_fp_offset = offset + sp_offset;
> > -	mips_save_restore_reg (word_mode, regno, offset, fn);
> > -	offset -= UNITS_PER_WORD;
> > -      }
> >
> > +  if (!TARGET_MICROMIPS
> > +      || (TARGET_MICROMIPS
> > +          && !umips_build_save_restore (fn, offset, sp_offset)))
> > +    mips_save_restore_registers (GP_REG_LAST, GP_REG_FIRST,
> > +				 &offset, fn, sp_offset);
> > +
> 
> With the pass-back suggestion above, I think this should be:
> 
>   offset = cfun->machine->frame.gp_sp_offset - sp_offset;
>   mask = cfun->machine->frame.mask
>   if (TARET_MICROMIPS16)
>     umips_build_save_restore (fn, &mask, &offset);
>   for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
>     if (BITSET_P (mask, regno - GP_REG_FIRST))
>       {
> 	/* Record the ra offset for use by mips_function_profiler.  */
> 	if (regno == RETURN_ADDR_REGNUM)
> 	  cfun->machine->frame.ra_fp_offset = offset + sp_offset;
> 	mips_save_restore_reg (word_mode, regno, offset, fn);
> 	offset -= UNITS_PER_WORD;
>       }
> 
> > @@ -11078,6 +11357,7 @@ mips_deallocate_stack (rtx base, rtx offset,
> HOST_
> >      return;
> >
> >    mips_frame_barrier ();
> > +
> >    if (offset == const0_rtx)
> >      {
> >        emit_move_insn (stack_pointer_rtx, base); @@ -11196,9 +11476,9
> > @@ mips_expand_epilogue (bool sibcall_p)
> >   	if (BITSET_P (mask, regno - GP_REG_FIRST))
> >   	  {
> >   	    offset -= UNITS_PER_WORD;
> > - 	    mips_save_restore_reg (word_mode, regno, offset,
> mips_restore_reg);
> > + 	    mips_save_restore_single_reg (word_mode, regno,
> > +                                          offset, mips_restore_reg);
> >   	  }
> > -
> >        /* Restore the remaining registers and deallocate the final bit
> >  	 of the frame.  */
> >        mips_frame_barrier ();
> > @@ -11211,7 +11491,6 @@ mips_expand_epilogue (bool sibcall_p)
> >        mips_for_each_saved_acc (frame->total_size - step2,
> mips_restore_reg);
> >        mips_for_each_saved_gpr_and_fpr (frame->total_size - step2,
> >  				       mips_restore_reg);
> > -
> >        if (cfun->machine->interrupt_handler_p)
> >  	{
> >  	  HOST_WIDE_INT offset;
> 
> Please leave out these whitespace changes.  I'd also prefer not to add the
> _single_reg suffix to mips_save_restore_reg.
> 
> > @@ -11254,7 +11533,6 @@ mips_expand_epilogue (bool sibcall_p)
> >  	/* Deallocate the final bit of the frame.  */
> >  	mips_deallocate_stack (stack_pointer_rtx, GEN_INT (step2), 0);
> >      }
> > -  gcc_assert (!mips_epilogue.cfa_restores);
> >
> >    /* Add in the __builtin_eh_return stack adjustment.  We need to
> 
> Please don't remove the assert.  AIUI this was for epilogues that use
> JRADDIUSP, which isn't part of the initial patch.
> 
> > @@ -16211,17 +16489,19 @@ mips_output_mi_thunk (FILE *file, tree
> thunk_fndec
> >    reload_completed = 0;
> >  }
> >  

> > -/* The last argument passed to mips_set_mips16_mode, or negative if the
> > -   function hasn't been called yet.  */
> > +/* The last argument passed to mips_set_mips16_micromips_mode,
> > +   or negative if the function hasn't been called yet.  */
> >  static int was_mips16_p = -1;
> > +static int was_micromips_p = -1;
> >
> >  /* Set up the target-dependent global state so that it matches the
> >     current function's ISA mode.  */
> >
> >  static void
> > -mips_set_mips16_mode (int mips16_p)
> > +mips_set_mips16_micromips_mode (int mips16_p, int micromips_p)
> >  {
> > -  if (mips16_p == was_mips16_p)
> > +  if (mips16_p == was_mips16_p
> > +      && micromips_p == was_micromips_p)
> >      return;
> >
> >    /* Restore base settings of various flags.  */
> 
> Now that we represent the mode using masks of MASK_MIPS16 and
> MASK_MICROMIPS, I think it would be better to have a mask rather than
> two booleans here too.
> I.e.:
> 
> /* The last argument passed to mips_set_mips16_micromips_mode,
>    or negative if the function hasn't been called yet.  */ static unsigned int
> old_compression_mode = -1;
> 
> /* Set up the target-dependent global state for ISA mode
> COMPRESSION_MODE,
>    which is either MASK_MIPS16 or MASK_MICROMIPS.  */
> 
> static void
> mips_set_compression_mode (unsigned int compression_mode)
> 
> > @@ -16290,6 +16570,18 @@ static void
> >        /* Switch to normal (non-MIPS16) mode.  */
> >        target_flags &= ~MASK_MIPS16;
> >
> > +      if (micromips_p)
> > +	{
> > +	  /* Switch to microMIPS mode.  */
> > +	  target_flags |= MASK_MICROMIPS;
> > +
> > +	  /* Avoid branch likely.  */
> > +	  target_flags &= ~MASK_BRANCHLIKELY;
> > +	}
> > +      else
> > +	/* Switch to normal (non-microMIPS) mode.  */
> > +	target_flags &= ~MASK_MICROMIPS;
> > +
> 
> With the change above, I think we should have:
> 
>   target_flags &= ~(MASK_MIPS16 | MASK_MICROMIPS);
>   target_flags |= compression_mode;
> 
> outside of the "if" statement, with the code above becoming:
> 
>     /* There are no microMIPS branch-likely instructions.  */
>     if (TARGET_MICROMIPS)
>       target_flags &= ~MASK_BRANCHLIKELY;
> 
> > +  /* Save the base compression state and process flags as though we
> > +     are generating uncompressed code.  */  if (TARGET_MIPS16)
> > +    mips_base_compression_flags |= MASK_MIPS16;  if
> > + (TARGET_MICROMIPS)
> > +    mips_base_compression_flags |= MASK_MICROMIPS;
> > +
> > +  target_flags &= ~MASK_MIPS16;
> > +  target_flags &= ~MASK_MICROMIPS;
> 
> Would prefer:
> 
>   mips_base_compression_flags = TARGET_COMPRESSION;
>   target_flags &= ~TARGET_COMPRESSION;
> 
> > @@ -17088,6 +17397,316 @@ mips_mulsidi3_gen_fn (enum rtx_code
> ext_code)
> >        return signed_p ? gen_mulsidi3_32bit : gen_umulsidi3_32bit;
> >      }
> >  }
> > +
> > +
> > +/* Return true if PATTERN matches the kind of instruction generated by
> > +   micromips_build_save_restore.  SAVE_P is true for store.  */
> 
> umips_build_save_restore
> 
> > +
> > +bool
> > +umips_save_restore_pattern_p (bool save_p, rtx pattern) {
> > +  int n;
> > +  HOST_WIDE_INT first_offset = 0;
> > +  rtx first_base = 0;
> > +  unsigned int first_regno = 0;
> > +
> > +  for (n = 0; n < XVECLEN (pattern, 0); n++)
> > +    {
> > +      rtx set, reg, mem, this_base;
> > +      HOST_WIDE_INT this_offset;
> > +      unsigned int this_regno;
> > +
> > +      /* Check that we have a SET.  */
> > +      set = XVECEXP (pattern, 0, n);
> > +      if (GET_CODE (set) != SET)
> > +	return false;
> > +
> > +      /* Check that the SET is a load (if restoring) or a store
> > +	 (if saving).  */
> > +      mem = save_p ? SET_DEST (set) : SET_SRC (set);
> > +      if (!MEM_P (mem) || MEM_VOLATILE_P (mem))
> > +	return false;
> > +
> > +      /* Check that the address is the sum of base and a possibly-zero
> > +	 constant offset.  Determine if the offset is in range.  */
> > +      mips_split_plus (XEXP (mem, 0), &this_base, &this_offset);
> > +      if (!REG_P (this_base)
> > +	  && UMIPS_12BIT_OFFSET_P (this_offset))
> > +	return false;
> > +
> > +      if (n == 0)
> > +	{
> > +	  first_base = this_base;
> > +	  first_offset = this_offset;
> > +	}
> 
> The UMIPS_12BIT_OFFSET_P check should be in the "n == 0" arm.
> 
> > +      else
> > +	{
> > +	  /* Check if this_base is the same as first_base.  */
> > +	  if (REGNO (this_base) != REGNO (first_base))
> > +	    return false;
> > +
> > +	  /* Check if this_offset is first_offset + UNITS_PER_WORD * n.  */
> > +	  if (this_offset != first_offset + UNITS_PER_WORD * n)
> > +	    return false;
> 
>       /* Check that the save slots are consecutive.  */
>       if (REGNO (this_base) != REGNO (first_base)
> 	  || this_offset != first_offset + UNITS_PER_WORD * n)
> 
> > +      /* Make sure the order of regno is "$16-$23, $30, $31", "$16-$23, $30",
> > +	 or "$31".  */
> > +      this_regno = REGNO (reg);
> > +      if (n == 0)
> > +	{
> > +	  if (this_regno != 16 && this_regno != 31)
> > +	    return false;
> > +	  first_regno = this_regno;
> > +	}
> > +      else if (n == 8) /* For s8.  */
> > +	{
> > +	  if (n == XVECLEN (pattern, 0) - 1)
> > +	    {
> > +	      if (this_regno != 30 && this_regno != 31)
> > +		return false;
> > +	    }
> > +	  else
> > +	    {
> > +	      if (this_regno != 30)
> > +		return false;
> > +	    }
> > +	}
> > +      else if (n != XVECLEN (pattern, 0) - 1)
> > +	{
> > +	  if (this_regno != first_regno + n)
> > +	    return false;
> > +	}
> > +      else /* The last item.  */
> > +	{
> > +	  if ((this_regno != first_regno + n) && this_regno != 31)
> > +	    return false;
> > +	}
> > +    }
> 
> Please instead check that the registers are in ascending order, build a mask,
> and then check that mask against umips_swm_mask at the end.
> 
> > +}
> > +/* Return true if MEM1 and MEM2 use the same base register, and the
> 
> Missing space between functions.
> 
> > +   offset of MEM2 equals the offset of MEM1 plus 4.  FIRST_REG is the
> > +   register into (from) which the contents of MEM1 will be loaded
> > +   (stored), depending on the value of LOAD_P.
> > +   SWAP_P is true when the 1st and 2nd instructions are swapped.  */
> > +
> > +static bool
> > +umips_load_store_pair_p_1 (bool load_p, bool swap_p, rtx first_reg,
> > +			   rtx mem1, rtx mem2)
> > +{
> > +  rtx base1, base2;
> > +  HOST_WIDE_INT offset1, offset2;
> > +
> > +  if (!MEM_P (mem1) || !MEM_P (mem2))
> > +    return false;
> > +
> > +  /* Make sure memory is base plus offset.  */  if (GET_CODE (XEXP
> > + (mem1, 0)) != PLUS
> > +      || GET_CODE (XEXP (mem2, 0)) != PLUS
> > +      || GET_CODE (XEXP (XEXP (mem1, 0), 1)) != CONST_INT
> > +      || GET_CODE (XEXP (XEXP (mem2, 0), 1)) != CONST_INT)
> > +    return false;
> > +
> > +  mips_split_plus (XEXP (mem1, 0), &base1, &offset1);
> > + mips_split_plus (XEXP (mem2, 0), &base2, &offset2);
> 
> Please remove the:
> 
>   /* Make sure memory is base plus offset.  */
>   if (GET_CODE (XEXP (mem1, 0)) != PLUS
>       || GET_CODE (XEXP (mem2, 0)) != PLUS
>       || GET_CODE (XEXP (XEXP (mem1, 0), 1)) != CONST_INT
>       || GET_CODE (XEXP (XEXP (mem2, 0), 1)) != CONST_INT)
>     return false;
> 
> part.  The point of using mips_split_plus is that we want to handle
> (reg) paired with (plus (reg) (const_int 4)) and (plus (reg) (const_int -4))
> paired with (reg).
> 
> > +  if (!REG_P (base1) || !rtx_equal_p (base1, base2))
> > +    return false;
> > +
> > +  /* Avoid invalid load pair instructions.  */  if (load_p && REGNO
> > + (first_reg) == REGNO (base1))
> > +    return false;
> > +
> > +  /* We must avoid this case for anti-dependence.
> > +     Ex:  lw $3, 4($3)
> > +          lw $2, 0($3)
> > +     first_reg is $2, but the base is $3.  */  if (load_p && swap_p
> > + && (REGNO (first_reg) + 1) == REGNO (base1))
> > +    return false;
> 
> Redundant brackets around "REGNO (first_reg) + 1".
> 
> > +
> > +  if (swap_p
> > +      && offset2 + 4 != offset1)
> > +    return false;
> > +
> > +  if (offset1 + 4 != offset2)
> > +    return false;
> 
> This always returns false for swap_p.  Please add tests for both cases to
> make sure that the code is triggering.  E.g. you could have an inline asm that
> takes two asm register variables, one in $2 and one in $3.
> 
> > +bool
> > +umips_load_store_pair_p (bool load_p, rtx *operands)
> 
> Function needs a comment.
> 
> > +{
> > +  rtx reg1, reg2, mem1, mem2;
> > +
> > +  if (load_p)
> > +    {
> > +      reg1 = operands[0];
> > +      reg2 = operands[2];
> > +      mem1 = operands[1];
> > +      mem2 = operands[3];
> > +    }
> > +  else
> > +    {
> > +      reg1 = operands[1];
> > +      reg2 = operands[3];
> > +      mem1 = operands[0];
> > +      mem2 = operands[2];
> > +    }
> > +
> > +  if (REGNO (reg2) == REGNO (reg1) + 1)
> > +      return umips_load_store_pair_p_1 (load_p, false, reg1, mem1,
> > + mem2);
> > +
> > +  else if (REGNO (reg1) == REGNO (reg2) + 1)
> > +      return umips_load_store_pair_p_1 (load_p, true, reg2, mem1,
> > + mem2);
> > +
> > +  return false;
> 
> Excess indentation.  Probably better without the "else", since this is really
> early-exit style.
> 
> > +/* Return the assembly instruction for microMIPS lwp or swp.
> > +   LOAD_P is true for load.  */
> > +
> > +static void
> > +umips_output_load_store_pair_1 (bool load_p, rtx reg, rtx mem) {
> > +  HOST_WIDE_INT offset;
> > +  rtx base, local_ops[3];
> > +
> > +  gcc_assert (REG_P (reg) && MEM_P (mem));
> > +
> > +  mips_split_plus (XEXP (mem, 0), &base, &offset);  gcc_assert (REG_P
> > + (base));
> > +
> > +  local_ops[0] = reg;
> > +  local_ops[1] = gen_rtx_CONST_INT (Pmode, offset);
> > +  local_ops[2] = base;
> > +  if (load_p)
> > +    output_asm_insn ("lwp %0,%1(%2)", local_ops);
> > +  else
> > +    output_asm_insn ("swp %0,%1(%2)", local_ops); }
> 
> This ought to be equivalent to:
> 
>   rtx ops[] = { reg, mem };
> 
>   if (load_p)
>     output_asm_insn ("lwp %0,%1", ops);
>   else
>     output_asm_insn ("swp %0,%1", ops);
> 
> > +/* Return true if reg1 and reg2 can be target of movep.  */
> > +
> > +bool
> > +umips_movep_target_p (rtx reg1, rtx reg2) {
> > +  int regno1, regno2, pair;
> > +  unsigned int i;
> > +  static const int match[8] = {0x00000060,	/* 5, 6 */
> > +			       0x000000a0,	/* 5, 7 */
> > +			       0x000000c0,	/* 6, 7 */
> > +			       0x00200010,	/* 4, 21 */
> > +			       0x00400010,	/* 4, 22 */
> > +			       0x00000030,	/* 4, 5 */
> > +			       0x00000050,	/* 4, 6 */
> > +			       0x00000090};	/* 4, 7 */
> 
> Should be formatted as:
> 
>   static const int match[8] = {
>     0x00000060,	/* 5, 6 */
>     ...
>   };
> 
> > +  for (i = 0; i < ARRAY_SIZE (match); i++)
> > +    {
> > +      if (pair == match[i])
> > +	return true;
> > +    }
> 
> Redundant { ... }.
> 
> > @@ -2452,18 +2459,48 @@ typedef struct mips_args {
> >     all calls should use assembly macros.  Otherwise, all indirect
> >     calls should use "jr" or "jalr"; we will arrange to restore $gp
> >     afterwards if necessary.  Finally, we can only generate direct
> > -   calls for -mabicalls by temporarily switching to non-PIC mode.  */
> > +   calls for -mabicalls by temporarily switching to non-PIC mode.
> > +
> > +   For microMIPS jal(r), we try to generate jal(r)s when a 16-bit
> > +   instruction is in the delay slot of jal(r).  */
> >  #define MIPS_CALL(INSN, OPERANDS, TARGET_OPNO, SIZE_OPNO)	\
> > +  (TARGET_MICROMIPS						\
> > +   ? (TARGET_USE_GOT && !TARGET_EXPLICIT_RELOCS
> 	\
> > +      ? (TARGET_INTERLINK_UNCOMPRESSED				\
> > +	 ? "%*" INSN "%!\t%" #TARGET_OPNO "%/"			\
> > +	 : "%*" INSN "\t%" #TARGET_OPNO "%/")			\
> 
> Is this special case really necessary?  Normally when using assembly macros
> we should leave the choice up to the assembler.  On the one hand we gain
> by having knowledge that the target is microMIPS, while on the other we
> lose because the assembler can't fill the delay slot with a 32-bit instruction
> from a preceding assembly macro.
> 
> Hopefully no-one really uses -mno-explicit-relocs these days anyway, so I
> think this case should be handled in the same way for all modes.
> 
> I.e.:
> 
>   TARGET_USE_GOT && !TARGET_EXPLICIT_RELOCS			\
>   ? "%*" INSN "\t%" #TARGET_OPNO "%/"				\
>   : REG_P (OPERANDS[TARGET_OPNO])				\
>   ? (mips_get_pic_call_symbol (OPERANDS, SIZE_OPNO)		\
>      ? ("%*.reloc\t1f,R_MIPS_JALR,%" #SIZE_OPNO "\n"		\
>         "1:\t" INSN "r\t%" #TARGET_OPNO "%/")			\
>      : TARGET_MICROMIPS && TARGET_INTERLINK_UNCOMPRESSED        \
>      ? "%*" INSN "r%!\t%" #TARGET_OPNO "%/"			\
>      : "%*" INSN "r\t%" #TARGET_OPNO "%/")			\
>   : MIPS_ABSOLUTE_JUMP ("%*" INSN "\t%" #TARGET_OPNO "%/")))
> 
> if that works.
> 
> > +

> > +/* Similar to MIPS_CALL, but this is for MICROMIPS "j" to generate
> > +   "jrc" when nop is in the delay slot of "jr".  */
> > +#define MICROMIPS_J(OPERANDS, OPNO)				\
> >    (TARGET_USE_GOT && !TARGET_EXPLICIT_RELOCS			\
> > -   ? "%*" INSN "\t%" #TARGET_OPNO "%/"				\
> > -   : (REG_P (OPERANDS[TARGET_OPNO])				\
> > -      && mips_get_pic_call_symbol (OPERANDS, SIZE_OPNO))	\
> > -   ? ("%*.reloc\t1f,R_MIPS_JALR,%" #SIZE_OPNO "\n"		\
> > -      "1:\t" INSN "r\t%" #TARGET_OPNO "%/")			\
> > -   : REG_P (OPERANDS[TARGET_OPNO])				\
> > -   ? "%*" INSN "r\t%" #TARGET_OPNO "%/"				\
> > -   : MIPS_ABSOLUTE_JUMP ("%*" INSN "\t%" #TARGET_OPNO "%/"))
> > -

> > +   ? "%*j\t%" #OPNO "%/"					\
> > +   : REG_P (OPERANDS[OPNO])					\
> > +   ? "%*jr%:\t%" #OPNO						\
> > +   : TARGET_ABICALLS && flag_pic				\
> > +   ? (".option\tpic0\n\t"					\
> > +      "%*j\t%" #OPNO "%/\n\t"					\
> > +      ".option\tpic2")						\
> > +   : "%*j\t%" #OPNO "%/")
> 
> Looks like the last bit could/should use MIPS_ABSOLUTE_JUMP.
> 
> > @@ -2923,3 +2960,5 @@ extern GTY(()) struct target_globals
> *mips16_globa
> >     with arguments ARGS.  */
> >  #define PMODE_INSN(NAME, ARGS) \
> >    (Pmode == SImode ? NAME ## _si ARGS : NAME ## _di ARGS)
> > +
> > +#define UMIPS_12BIT_OFFSET_P(OFFSET) (IN_RANGE (OFFSET, -2048,
> 2047))
> 
> Please put this after #define LUI_INT, to keep the range checks together.
> 
> Thanks,
> Richard

Attachment: gcc.cl
Description: gcc.cl

Attachment: gcc.patch
Description: gcc.patch

Attachment: libgcc.patch
Description: libgcc.patch

Attachment: libgcc.cl
Description: libgcc.cl

Attachment: gcc-testsuite.patch
Description: gcc-testsuite.patch

Attachment: gcc-testsuite.cl
Description: gcc-testsuite.cl


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]