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


Thanks, this is looking much better now.

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> 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.

Hmm, to be honest, I'd rather you kept it local to the Mentor toolchain.
The way the patch is written, we (rightly) use JALS even with
-minterlink-compressed in cases where we can prove that the call
is to another microMIPS function, so calling -mno-jals an alias of
-minterlink-compressed would be a bit confusing.

-mno-jals sounds like it ought to disable JALS in all situations,
which would need special code to handle.  I think doing that would be
extra baggage with little benefit.

> Index: gcc.target/mips/umips-constraints-1.c
> ===================================================================
> --- gcc.target/mips/umips-constraints-1.c	(revision 0)
> +++ gcc.target/mips/umips-constraints-1.c	(revision 0)
> @@ -0,0 +1,14 @@
> +/* { dg-options "(-mmicromips)" } */
> +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> +
> +MICROMIPS void
> +foo (int *x)
> +{
> +  asm volatile ("insn1\t%a0" :: "ZD" (&x[0]));
> +  asm volatile ("insn2\t%a0" :: "ZD" (&x[511]));
> +  asm volatile ("insn3\t%a0" :: "ZD" (&x[512]));
> +}
> +
> +/* { dg-final { scan-assembler "\tinsn1\t0\\(" } } */
> +/* { dg-final { scan-assembler "\tinsn2\t2044\\(" } } */
> +/* { dg-final { scan-assembler "\tinsn3\t2048\\(" } } */

But the insn3 is wrong, isn't it?  I suggested scan-assembler-not for
that one because I thought it had to be a 12-bit signed offset on
microMIPS.  The patch has:

(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{ZD} 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)")))

but it looks like it ought to be:

   (if_then_else (match_test "TARGET_MICROMIPS")
	         (match_test "umips_12bit_offset_address_p (op, mode)")
	         (match_test "mips_address_insns (op, mode, false)"))

Same for ZC.

> @@ -1246,6 +1247,10 @@ proc mips-dg-options { args } {
>  	append extra_tool_flags " -DMIPS16=__attribute__((mips16))"
>      }
>  
> +    if { [mips_have_test_option_p options "-mmicromips"] } {
> +	append extra_tool_flags " -DMICROMIPS=__attribute__((micromips))"
> +    }
> +
>      # Use our version of gcc-dg-test for this test.
>      if { ![string equal [info procs "mips-gcc-dg-test"] ""] } {
>  	rename gcc-dg-test mips-old-gcc-dg-test
> @@ -1275,6 +1280,6 @@ proc mips-gcc-dg-test { prog do_what extra_tool_fl
>  dg-init
>  mips-dg-init
>  gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] \
> -    "-DNOMIPS16=__attribute__((nomips16))"
> +    "-DNOMIPS16=__attribute__((nomips16)) -DNOMICROMIPS=__attribute__((nomicromips)) -DNOCOMPRESSION=__attribute__((nocompression)) -DMICROMIPS=__attribute__((micromips))"
>  mips-dg-finish
>  dg-finish

Please drop the -DMICROMIPS in the second hunk.  The first hunk is the
right way to add it.  The idea is that we want a compilation failure if
the test uses MICROMIPS but forgets to add:

/* { dg-options "(-mmicromips)" } */

The -DNOMICROMIPS and -DNOCOMPRESSION in the second hunk are fine though.

> +/* { dg-options "(-mmicromips)" } */
> +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" "-O1" } { "" } } */

Sorry, only just realised you were skipping -O1.  Please add a comment saying
why it fails at -O1 or (preferably) add whichever other option is needed for
the test to pass at -O1.  I assume it's -fpeephole2 in this case.

Same for all tests with this skip.

> Index: gcc.target/mips/umips-lwp-swp-2.c
> ===================================================================
> --- gcc.target/mips/umips-lwp-swp-2.c	(revision 0)
> +++ gcc.target/mips/umips-lwp-swp-2.c	(revision 0)
> @@ -0,0 +1,19 @@
> +/* { dg-options "-mgp32 (-mmicromips)" } */
> +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" "-O1" "-Os" } { "" } } */
> +int a[2];
> +
> +MICROMIPS f (b)
> +{
> +  unsigned int i;
> +  int *p;
> +  for (p = &a[b], i = b; --i < ~0; )
> +    *--p = i * 3 + (int)a;
> +
> +}
> +
> +MICROMIPS main ()
> +{
> +  a[0] = a[1] = 0;
> +  f (2);
> +}
> +/* { dg-final { scan-assembler "\tswp\t\\\$3,0\\(\\\$3\\)" } }*/

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?

> Index: gcc.target/mips/umips-lwp-swp-volatile.c
> ===================================================================
> --- gcc.target/mips/umips-lwp-swp-volatile.c	(revision 0)
> +++ gcc.target/mips/umips-lwp-swp-volatile.c	(revision 0)
> @@ -0,0 +1,41 @@
> +/* { dg-do compile } */
> +
> +/* This test ensures that we do not generate microMIPS SWP or LWP
> +   instructions when any component of the accessed memory is volatile; 
> +   they are unsafe for such since they might cause replay of partial 
> +   accesses if interrupted by an exception.  */
> +
> +static void set_csr (volatile void *p, int v)
> +{
> +  *(volatile int *) (p) = v;
> +}
> +
> +static int get_csr (volatile void *p)
> +{
> +  return *(volatile int *) (p);
> +}
> +
> +int main ()
> +{
> +  int i, q = 0, p = 0, r = 0;
> +
> +  for (i = 0; i < 20; i++)
> +    {
> +      set_csr ((volatile void *) 0xbf0100a8, 0xffff0002);
> +      set_csr ((volatile void *) 0xbf0100a4, 0x80000008);
> +    }
> +
> +  for (i = 0; i < 20; i++)
> +    {
> +      register int k, j;
> +      k = get_csr ((volatile void *) 0xbf0100b8);
> +      p += k;
> +      j = get_csr ((volatile void *) 0xbf0100b4);
> +      r += j;
> +      q = j + k;
> +    }
> +  return q + r + p;
> +}
> +
> +/* { dg-final { scan-assembler-not "\tswp" } } */
> +/* { dg-final { scan-assembler-not "\tlwp" } } */

Please add:

  /* { dg-options "-mmicromips" } */

here.  ("-mmicromips" rather than "(-mmicromips)" because this is one
test where I think it makes more sense to force microMIPS mode throughout
the whole file.)

> Index: doc/md.texi
> ===================================================================
> --- doc/md.texi	(revision 195900)
> +++ doc/md.texi	(working copy)
> @@ -2916,6 +2916,19 @@ Floating-point zero.
>  
>  @item R
>  An address that can be used in a non-macro load or store.
> +
> +@item ZC
> +When compiling microMIPS code, this constraing matches a memory operand

Missing the s/contraing/constraint/ fix mentioned last time.  (You fixed
it in constraints.md though, thanks.)

> +(define_insn "*swp"
> +  [(parallel [(set (match_operand:SI 0 "non_volatile_mem_operand")
> +		   (match_operand:SI 1 "d_operand"))
> +	      (set (match_operand:SI 2 "non_volatile_mem_operand")
> +		   (match_operand:SI 3 "d_operand"))])]
> +
> +  "TARGET_MICROMIPS
> +   && umips_load_store_pair_p (false, operands)"
> +{
> +  umips_output_load_store_pair (false, operands);
> +  return "";
> +}
> +  [(set_attr "type" "store")
> +   (set_attr "mode" "SI")
> +   (set_attr "can_delay" "no")])

;; The behavior of the SWP insn is undefined if placed in a delay slot.

> @@ -5447,11 +5464,20 @@
>  	 (pc)))]
>    "!TARGET_MIPS16"
>  {
> +  /* For a simple BNEZ or BEQZ microMIPS branch.  */
> +  if (TARGET_MICROMIPS
> +      && operands[3] == const0_rtx
> +      && get_attr_length (insn) <= 8)
> +    return mips_output_conditional_branch (insn, operands,
> +					   "%*b%C1z%:\t%2,%0",
> +					   "%*b%N1z%:\t%2,%0");
> +    
>    return mips_output_conditional_branch (insn, operands,
>  					 MIPS_BRANCH ("b%C1", "%2,%z3,%0"),
>  					 MIPS_BRANCH ("b%N1", "%2,%z3,%0"));
>  }
> -  [(set_attr "type" "branch")])
> +  [(set_attr "type" "branch")
> +   (set_attr "mode" "none")])
>  

Minor, sorry, but please don't add the mode attribute.  (You didn't add
it to the inverted case, thanks.)

>    if (TREE_CODE (decl) != FUNCTION_DECL)
>      {
> -      if (mips16_p)
> -	error ("%qs attribute only applies to functions", "mips16");
> -      if (nomips16_p)
> -	error ("%qs attribute only applies to functions", "nomips16");
> +      if (nocompression_flags)
> +	error ("%qs attribute only applies to function",
> +	       mips_get_compress_off_name (nocompression_flags));
> +
> +      if (compression_flags)
> +	error ("%qs attribute only applies to function",
> +	       mips_get_compress_on_name (nocompression_flags));

Sorry, didn't notice this before, but we're losing the plural here.
The old error message (with "functions") was correct.

>      }
>    else
>      {
> -      mips16_p |= mips_mips16_decl_p (decl);
> -      nomips16_p |= mips_nomips16_decl_p (decl);
> -      if (mips16_p || nomips16_p)
> +      compression_flags |= mips_get_compress_on_flags (DECL_ATTRIBUTES (decl));
> +      nocompression_flags |=
> +	mips_get_compress_off_flags (DECL_ATTRIBUTES (decl));
> +
> +      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_flags & MASK_MIPS16
> +          && compression_flags & MASK_MICROMIPS)
> +	error ("%qE cannot have both %s and %s attributes",
> +	       DECL_NAME (decl), "mips16", "micromips");

The first error message is right, but the second one should be the same
("%qs" instead of "%s", so that the name gets quoted).

> +
> +      if (TARGET_FLIP_MIPS16
> +	  && !DECL_ARTIFICIAL (decl)
> +	  && compression_flags == 0
> +	  && nocompression_flags == 0)
>  	{
> -	  /* DECL cannot be simultaneously "mips16" and "nomips16".  */
> -	  if (mips16_p && nomips16_p)
> -	    error ("%qE cannot have both %<mips16%> and "
> -		   "%<nomips16%> attributes",
> -		   DECL_NAME (decl));
> -	}
> -      else if (TARGET_FLIP_MIPS16 && !DECL_ARTIFICIAL (decl))
> -	{
>  	  /* Implement -mflip-mips16.  If DECL has neither a "nomips16" nor a
>  	     "mips16" attribute, arbitrarily pick one.  We must pick the same
>  	     setting for duplicate declarations of a function.  */
>  	  name = mflip_mips16_use_mips16_p (decl) ? "mips16" : "nomips16";
> +	  name = "nomips16";
> + 	  *attributes = tree_cons (get_identifier (name), NULL, *attributes);
> +	  name = "nomicromips";
>  	  *attributes = tree_cons (get_identifier (name), NULL, *attributes);

As I mentioned last time, this unconditionally overrides the first "name"
assignment.  I think it should be correct with:

	  name = mflip_mips16_use_mips16_p (decl) ? "mips16" : "nomips16";
	  *attributes = tree_cons (get_identifier (name), NULL, *attributes);
	  name = "nomicromips";
	  *attributes = tree_cons (get_identifier (name), NULL, *attributes);

> +/* Return true if a call to DECL may need to use JALX.  */
> +
> +static bool
> +mips_call_may_need_jalx_p (tree 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;
> +
> +  /* 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
> +	       (TYPE_ATTRIBUTES (TREE_TYPE (decl))) ==0)
> +	return true;
> +      if (TARGET_COMPRESSION
> +	  && mips_get_compress_on_flags
> +	       (TYPE_ATTRIBUTES (TREE_TYPE (decl))) == 0)

This should be DECL_ATTRIBUTES (decl) rather than TYPE_ATTRIBUTES (...):

      if (!TARGET_COMPRESSION
	  && mips_get_compress_off_flags (DECL_ATTRIBUTES (decl)) == 0)
	return true;
      if (TARGET_COMPRESSION
	  && mips_get_compress_on_flags (DECL_ATTRIBUTES (decl)) == 0)
	return true;

> +/* Capture the register combinations that are allowed in a SWM or LWM
> +   instruction.  The entries are ordered by number of registers set in
> +   the mask.  We also ignore the single register encodings because a
> +   normal SW/LW is preferred.  */
> +   
> +static const unsigned int umips_swm_mask[17] = {
> +  0xc0ff0000, 0x80ff0000, 0x40ff0000, 0x807f0000,
> +  0x00ff0000, 0x803f0000, 0x007f0000, 0x801f0000,	
> +  0x003f0000, 0x800f0000, 0x001f0000, 0x80070000,
> +  0x000f0000, 0x80030000, 0x00070000, 0x80010000,
> +  0x00030000 };
> +
> +static const unsigned int umips_swm_encoding[17] = {
> +  25, 24, 9, 23, 8, 22, 7, 21, 6, 20, 5, 19, 4, 18, 3, 17, 2 };

You missed the bit from last time about "};" being on its own line:

static const unsigned int umips_swm_mask[17] = {
  0xc0ff0000, 0x80ff0000, 0x40ff0000, 0x807f0000,
  0x00ff0000, 0x803f0000, 0x007f0000, 0x801f0000,	
  0x003f0000, 0x800f0000, 0x001f0000, 0x80070000,
  0x000f0000, 0x80030000, 0x00070000, 0x80010000,
  0x00030000
};

static const unsigned int umips_swm_encoding[17] = {
  25, 24, 9, 23, 8, 22, 7, 21, 6, 20, 5, 19, 4, 18, 3, 17, 2
};

> @@ -16437,14 +16693,19 @@ mips_option_override (void)
>    if (global_options_set.x_mips_isa_option)
>      mips_isa_option_info = &mips_cpu_info_table[mips_isa_option];
>  
> -  /* Process flags as though we were generating non-MIPS16 code.  */
> -  mips_base_mips16 = TARGET_MIPS16;
> -  target_flags &= ~MASK_MIPS16;
> -
>  #ifdef SUBTARGET_OVERRIDE_OPTIONS
>    SUBTARGET_OVERRIDE_OPTIONS;
>  #endif
>  
> +  /* MIPS16 and microMIPS cannot coexist  */

Missing period ("/* MIPS16 and microMIPS cannot coexist.  */")

> +  if (TARGET_MICROMIPS && TARGET_MIPS16)
> +    error ("unsupported combination: %s", "-mips16 -mmicromips");
> +
> +  /* Save the base compression state and process flags as though we
> +     are generating uncompressed code.  */
> +  mips_base_compression_flags = TARGET_COMPRESSION;
> +  target_flags &= ~TARGET_COMPRESSION;
> +
>    /* -mno-float overrides -mhard-float and -msoft-float.  */
>    if (TARGET_NO_FLOAT)
>      {

s/as though we are/as though we were/ (as in the original comment).

> +/* Return true if MEM1 and MEM2 use the same base register, and the
> +   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, rtx first_reg, rtx mem1, rtx mem2)

We still need the swap_p paramter (which is still mentioned in the comment),
because we want to test it here:

> +  /* 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
> +      && REGNO (first_reg) + 1 == REGNO (base1))
> +    return false;

The old code was right about that, it was the offset stuff that seemed wrong.

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.

Richard


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