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

["Moore, Catherine" <Catherine_Moore at mentor dot com>]

Hi Richard,

I've now attached updated patches.  WDYT?  Are we ready to commit?

I'd also like to support -mno-jals for backward compatibility.  Are you okay with that?  If so, I'll submit as a separate patch.

Thanks,
Catherine

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Wednesday, February 13, 2013 6:43 AM
> 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:
> >> > 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)
> 
> Thanks for doing this, but...
> 
> >> 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.
> 
> ...it looks like you haven't addressed this part.  What I mean is:
> the stack alignment is 8 bytes rather than 16 bytes on 32-bit ABIs, so I would
> have expected 0x7f8 rather than 0x7f0, as for the MIPS16 case quoted
> above.  I wasn't sure why you went for 0x7f0 instead.
> 
> I'm not saying it must be 0x7f8, just that the comment ought to give the
> reason for using 0x7f0 instead.

I've now changed the constant to 0x7f8.  0x7f0 was used in the initial port, but I couldn't find the reason.  Testing with 0x7f8 looks fine.

> 
> >> > +      /* 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.
> 
> It looks like you haven't addressed this part.  Why do we need to add tree-
> level "nomips16" and "nomicromips" attributes to a function that has
> "nocompression"?

Now removed.
> 
> >> > +	  && 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.
> 
> This too.  The point is that addr.offset is an rtx, while
> UMIPS_12BIT_OFFSET_P takes a HOST_WIDE_INT:

Now done.

> Please add tests that ZC and ZD work for microMIPS.  E.g. the ZC one could be
> something like:
> 
Now done.
> 
> > +@item -minterlink-compressed
> > +@item -mno-interlink-compressed
> > +@item -minterlink-uncompressed
> > +@item -mno-interlink-uncompressed
> > +@opindex minterlink-compressed
> > +@opindex 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, and vice
> versa.
> 
> What I meant in my previous reply was that there should be only one set of
> options.  I.e. everything would use -minterlink-compressed and -mno-
> interlink-compressed.  There would be no -minterlink-uncompressed/ -mno-
> interlink-uncompressed
> 
> Sorry for going back on the earlier -mno-jals suggestion here, but I hadn't
> realised until I saw the previous patch just how confusing it would be to have
> both.
> 
That's okay -- I think I've now captured your intent.  I wasn't sure from your earlier reply if you meant to remove the no-interlink-uncompressed altogether.  I do agree that this approach is less confusing.

--Lots of comments addressed and snipped 

> > Index: Makefile.in
> >
> ==========================================================
> =========
> > --- Makefile.in	(revision 195931)
> > +++ Makefile.in	(working copy)
> > @@ -223,7 +223,7 @@
> >  # Options to use when compiling libgcc2.a.
> >  #
> >  LIBGCC2_DEBUG_CFLAGS = -g
> > -LIBGCC2_CFLAGS = -O2 $(LIBGCC2_INCLUDES) $(GCC_CFLAGS)
> > $(HOST_LIBGCC2_CFLAGS) \
> > +LIBGCC2_CFLAGS = $(LIBGCC2_INCLUDES) $(GCC_CFLAGS)
> > +$(HOST_LIBGCC2_CFLAGS) \
> >  		 $(LIBGCC2_DEBUG_CFLAGS) -DIN_LIBGCC2 \
> >  		 -fbuilding-libgcc -fno-stack-protector \
> >  		 $(INHIBIT_LIBC_CFLAGS)
> 
> Looks like this snuck in accidentally.  Please make sure that the patch works
> without it. :-)
Yep, sorry.  Tested with -O2.
> 
> I notice you're changing mips-sde-elf; that'd be a good target to make sure
> that the testsuite is still 64-bit clean.  E.g. one run with -mips64r2 (and
> without -mmicromips).
Also done.

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

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

Attachment: libgcc.cl
Description: libgcc.cl

Attachment: libgcc.patch
Description: libgcc.patch

Attachment: gcc.cl
Description: gcc.cl

Attachment: gcc.patch
Description: gcc.patch