FW: [PATCH] [MIPS] microMIPS gcc support

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
>> Looks good otherwise, but please post an updated patch.  It's probably
>> stating the obvious, but the patch is too invasive for this late stage
>> of 4.8, so
>> it'll need to wait until 4.9.
>> 
>
> The updated patch is attached.  How's it looking this time?

Almost there. :-)

As I mentioned in an earlier reply, please always check for warnings.
If you're not bootstrapping on a MIPS host then instead configure the
cross compiler with --enable-werror-always, using a recent GCC as the
host compiler.  If the patch goes in with warnings, it'll break the
build for other people.

The warnings I could see are:

> +  for (n = 0; n < XVECLEN (pattern, 0); n++)
> +    {
> +      rtx set, reg, mem, this_base;
> +      HOST_WIDE_INT this_offset;
> +      unsigned int this_regno;

this_regno is an unused variable (thanks for making it unused though).

>  static bool
>  mips_function_ok_for_sibcall (tree decl, tree exp ATTRIBUTE_UNUSED)
>  {
> +
> +  unsigned int compression_mode = mips_get_compress_mode (decl);

compression_mode is unused.

> +static bool
> +umips_build_save_restore (mips_save_restore_fn fn,
> +			  unsigned *mask, HOST_WIDE_INT *offset)
> +{
> +  int i, nregs;
> +  unsigned int j;
> +  rtx pattern, set, reg, mem;
> +  HOST_WIDE_INT this_offset;
> +  rtx this_base;
> +
> +  /* Try matching $16 to $31 (s0 to ra).  */
> +  for (i = 0; i < ARRAY_SIZE (umips_swm_mask); i++)
> +    if ((*mask & 0xffff0000) == umips_swm_mask[i])
> +      break;
> +
> +  if (i == ARRAY_SIZE (umips_swm_mask))
> +    return false;

"i" needs to be unsigned to avoid a "comparison between signed and unsigned"
warning.

> +/* Return the assembly instruction for microMIPS lwm or swm.
> +   SAVE_P and PATTERN are as for umips_save_restore_pattern_p.  */
> +
> +const char *
> +umips_output_save_restore (bool save_p, rtx pattern)
> +{
> +  static char buffer[300];
> +  char *s;
> +  int n;
> +  HOST_WIDE_INT offset;
> +  rtx base, mem, set, reg, last_set, last_reg;
> +
> +  /* Parse the pattern.  */
> +  gcc_assert (umips_save_restore_pattern_p (save_p, pattern));
> +
> +  s = strcpy (buffer, save_p ? "swm\t" : "lwm\t");
> +  s += strlen (s);
> +  n = XVECLEN (pattern, 0);
> +
> +  set = XVECEXP (pattern, 0, 0);
> +  reg = save_p ? SET_SRC (set) : SET_DEST (set);

"reg" is unused and should be deleted.

In case it sounds otherwise, I didn't build the patch to try to catch you out.
I built it so that I could try to answer:

> > Is this a test of the swap_p case?  Thanks if so, but could you add a comment
> > saying where the SWP gets generated, and why the test needs to be skipped
> > at -O1 and -Os?
> > 
> Yes, this was the swap_p case.  But it turns out that umips-lpw-swp-1.c
> was also testing the swap_p case.  I've now dropped this test in favor
> of a different test that exercises the swap_p = false case.
> You had mentioned testing for explicit registers.  This is turning out
> to be problematic.  The Linux and ELF tool chains don't always use the
> same register set.  In addition, -O3 seems to expose more opportunities
> for generating these instructions so the code generated isn't consistent
> among optimization options.  I've changed it back to just testing for
> swp, but I'm open to other suggestions.

I've attached some tests below, both for LWP (which didn't seem to be
covered) and SWP.  They use things like multiplication to force a
particular load or store order in scheduled code, so that both swap_p
and !swap_p are covered (verified using both printfs and -fno-peephole2).
umips-lwp-7.c tests for:

  /* 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;

while umips-lwp-8.c is an example of something that would be pessimised
by not having the swap_p test above.

I also noticed a couple of other things while testing:

- ISA_HAS_LOAD_DELAY should be false for microMIPS, otherwise we'll try
  to insert nops after LWP on targets that default to -mips1, and ICE
  when we see that we have a double SET.

- In the testsuite, a -mmicromips test option should override an ambient
  -mips16, and vice versa.

- umips-movep-2.c fails on n64 because there the moves are DImode rather
  than SImode.  Since the initial patch is just for 32-bit, I forced
  -mgp32 for now.  Also, the function needs a MICROMIPS attribute.

There were various other minor formatting and comment nits that were probably
easier for me to fix than describe.  There was also some trailing whitespace.

The patch is OK for 4.9 with this one applied on top of it, provided that
it works with your setup of course.  Thanks for your patience!

Richard

FW: [PATCH] [MIPS] microMIPS gcc support

Commit Message

Comments

Patch