FW: [PATCH] [MIPS] microMIPS gcc support
Moore, Catherine
Catherine_Moore@mentor.com
Tue Feb 19 16:31:00 GMT 2013
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-testsuite.cl
Type: application/octet-stream
Size: 620 bytes
Desc: gcc-testsuite.cl
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20130219/d660a080/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-testsuite.patch
Type: application/octet-stream
Size: 8745 bytes
Desc: gcc-testsuite.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20130219/d660a080/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libgcc.cl
Type: application/octet-stream
Size: 296 bytes
Desc: libgcc.cl
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20130219/d660a080/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libgcc.patch
Type: application/octet-stream
Size: 2195 bytes
Desc: libgcc.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20130219/d660a080/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc.cl
Type: application/octet-stream
Size: 5345 bytes
Desc: gcc.cl
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20130219/d660a080/attachment-0004.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc.patch
Type: application/octet-stream
Size: 89508 bytes
Desc: gcc.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20130219/d660a080/attachment-0005.obj>
More information about the Gcc-patches
mailing list