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: [PATCH,MIPS] MIPS64r6 support


Sorry for the slow review.

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> The initial support for MIP64r6 is intentionally minimal to make review
> easier. Performance enhancements and use of new MIPS64r6 features will
> be introduced separately. The current patch makes no attempt to
> get the testsuite appropriately configured for MIPS64r6 as the existing
> structure relies on each MIPS revision being a superset of the previous.
> Recommendations on how to rework the mips.exp logic to cope with this
> would be appreciated.

Could you give an example of the kind of thing you mean?

If tests really do need r5 or earlier, we should enforce that in the
dg-options.  E.g. for conditional moves we should add an isa_rev
limit to the existing tests and add new ones with isa_rev>=6.

I suppose we'll need a way of specifying an isa_rev range, say
"isa_rev=2-5".  That should be a fairly localised change though.

The:

    # Handle dependencies between the pre-arch options and the arch option.
    # This should mirror the arch and post-arch code below.
    if { !$arch_test_option_p } {

block should start with something like:

	if (isa_rev >= 6
	    && ...tests for things r6 doesn't support..) {
	    if { $gp_size == 32 } {
		mips_make_test_option options "-mips32r2"
	    } else {
		mips_make_test_option options "-mips64r2"
	    }

And:

    if { $arch_test_option_p } {

should have a corresponding:

        if { $isa_rev > 5 } {
            ...force options that r6 doesn't support to off...
        }

before the unsets.
        
> diff --git a/gcc/config/mips/constraints.md b/gcc/config/mips/constraints.md
> index f6834fd..f93ae1c 100644
> --- a/gcc/config/mips/constraints.md
> +++ b/gcc/config/mips/constraints.md
> @@ -324,10 +324,14 @@ (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}."
> +   not compiling for microMIPS code, @code{ZD} is either equivalent to
> +   @code{p} or matches an address operand that is formed from a base register
> +   and a 9-bit offset, depending on ISA support."

Maybe just change the comment to:

  "An address suitable for a @code{prefetch} instruction, or for any other
instruction with the same addressing mode as @code{prefetch}."

perhaps going on to say what the microMIPS, r6 and other cases are,
if you think that's better.

You need to update md.texi too; it isn't "yet" automatic.

>     (if_then_else (match_test "TARGET_MICROMIPS")
>  		 (match_test "umips_12bit_offset_address_p (op, mode)")
> -		 (match_test "mips_address_insns (op, mode, false)")))
> +		 (if_then_else (match_test "ISA_HAS_PREFETCH_9BIT")
> +			(match_test "mipsr6_9bit_offset_address_p (op, mode)")
> +			(match_test "mips_address_insns (op, mode, false)"))))

Please use (cond ...) instead.

> diff --git a/gcc/config/mips/linux.h b/gcc/config/mips/linux.h
> index e539422..751623f 100644
> --- a/gcc/config/mips/linux.h
> +++ b/gcc/config/mips/linux.h
> @@ -18,8 +18,9 @@ along with GCC; see the file COPYING3.  If not see
>  <http://www.gnu.org/licenses/>.  */
>  
>  #define GLIBC_DYNAMIC_LINKER \
> -  "%{mnan=2008:/lib/ld-linux-mipsn8.so.1;:/lib/ld.so.1}"
> +  "%{mnan=2008|mips32r6|mips64r6:/lib/ld-linux-mipsn8.so.1;:/lib/ld.so.1}"
>  
>  #undef UCLIBC_DYNAMIC_LINKER
>  #define UCLIBC_DYNAMIC_LINKER \
> -  "%{mnan=2008:/lib/ld-uClibc-mipsn8.so.0;:/lib/ld-uClibc.so.0}"
> +  "%{mnan=2008|mips32r6|mips64r6:/lib/ld-uClibc-mipsn8.so.0;" \
> +  ":/lib/ld-uClibc.so.0}"

Rather than update all the specs like this, I think we should force
-mnan=2008 onto the command line for r6 using DRIVER_SELF_SPECS.
See e.g. MIPS_ISA_SYNCI_SPEC.

> diff --git a/gcc/config/mips/mips-protos.h b/gcc/config/mips/mips-protos.h
> index 0b32a70..9560506 100644
> --- a/gcc/config/mips/mips-protos.h
> +++ b/gcc/config/mips/mips-protos.h
> @@ -341,6 +341,7 @@ extern bool umips_load_store_pair_p (bool, rtx *);
>  extern void umips_output_load_store_pair (bool, rtx *);
>  extern bool umips_movep_target_p (rtx, rtx);
>  extern bool umips_12bit_offset_address_p (rtx, enum machine_mode);
> +extern bool mipsr6_9bit_offset_address_p (rtx, enum machine_mode);

Would prefer just mips_9bit..., since a 9-bit offset looks the same
for all ISA levels.   I suppose it should have been just mips_12bit... too.

Same for MIPSR6_9BIT_OFFSET_P.

> @@ -4186,6 +4230,46 @@ mips_rtx_costs (rtx x, int code, int outer_code, int opno ATTRIBUTE_UNUSED,
>  	}
>        *total = mips_zero_extend_cost (mode, XEXP (x, 0));
>        return false;
> +    case TRUNCATE:
> +      /* Costings for highpart multiplies.  */
> +      if (ISA_HAS_R6MUL
> +	  && (GET_CODE (XEXP (x, 0)) == ASHIFTRT
> +	      || GET_CODE (XEXP (x, 0)) == LSHIFTRT)
> +	  && CONST_INT_P (XEXP (XEXP (x, 0), 1))
> +	  && ((INTVAL (XEXP (XEXP (x, 0), 1)) == 32
> +	       && GET_MODE (XEXP (x, 0)) == DImode)
> +	      || (ISA_HAS_R6DMUL
> +		  && INTVAL (XEXP (XEXP (x, 0), 1)) == 64
> +		  && GET_MODE (XEXP (x, 0)) == TImode))
> +	  && GET_CODE (XEXP (XEXP (x, 0), 0)) == MULT
> +	  && ((GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 0)) == SIGN_EXTEND
> +	       && GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 1)) == SIGN_EXTEND)
> +	      || (GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 0)) == ZERO_EXTEND
> +		  && (GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 1))
> +		      == ZERO_EXTEND))))

Ouch :-/

> +	{
> +	  if (!speed)
> +	    *total = COSTS_N_INSNS (1) + 1;
> +	  else if (mode == DImode)
> +	    *total = mips_cost->int_mult_di;
> +	  else
> +	    *total = mips_cost->int_mult_si;
> +
> +	  /* Sign extension is free, zero extension costs for DImode when
> +	     on a 64bit core / when DMUL is present.  */
> +	  if (ISA_HAS_R6DMUL && GET_MODE (XEXP (x, 0)) == DImode)
> +	    {
> +	      if (GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 0)) == ZERO_EXTEND)
> +		*total += rtx_cost (XEXP (XEXP (XEXP (x, 0), 0), 0),
> +				    ZERO_EXTEND, 0, speed);
> +
> +	      if (GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 1)) == ZERO_EXTEND)
> +		*total += rtx_cost (XEXP (XEXP (XEXP (x, 0), 0), 1),
> +				    ZERO_EXTEND, 0, speed);
> +	    }
> +	  return true;

In the calls to rtx_cost, the outer code should be MULT rather than
ZERO_EXTEND.  Maybe:

           for (int i = 0; i < 2; ++i)
             {
               rtx op = XEXP (XEXP (XEXP (x, 0), 0), i);
               if (ISA_HAS_R6DMUL
                   && GET_CODE (op) == ZERO_EXTEND
                   && GET_MODE (op) == DImode)
                 *total += rtx_cost (op, MULT, i, speed);
               else
                 *total += rtx_cost (XEXP (op, 0), GET_CODE (op), 0, speed);
             }

so that we consistently add the costs of the operands.

> @@ -4969,17 +5053,32 @@ mips_emit_compare (enum rtx_code *code, rtx *op0, rtx *op1, bool need_eq_ne_p)
>      {
>        enum rtx_code cmp_code;
>  
> -      /* Floating-point tests use a separate C.cond.fmt comparison to
> -	 set a condition code register.  The branch or conditional move
> -	 will then compare that register against zero.
> +      /* Floating-point tests use a separate C.cond.fmt or CMP.cond.fmt
> +	 comparison to set a condition code register.  The branch or
> +	 conditional move will then compare that register against zero.

"condition code register" doesn't apply to CMP.  Maybe just "register"?

> @@ -5105,7 +5232,9 @@ mips_expand_conditional_trap (rtx comparison)
>  
>    mode = GET_MODE (XEXP (comparison, 0));
>    op0 = force_reg (mode, op0);
> -  if (!arith_operand (op1, mode))
> +  if (!arith_operand (op1, mode)
> +      || (!ISA_HAS_COND_TRAPI
> +	  && !register_operand (op1, mode)))
>      op1 = force_reg (mode, op1);

arith_operand isn't meaningful without ISA_HAS_COND_TRAPI, so:

  if (!(ISA_HAS_COND_TRAPI
        ? arith_operand (op1, mode)
        : register_operand (op1, mode)))
    op1 = force_reg (mode, op1);

> @@ -11753,6 +11893,10 @@ mips_hard_regno_mode_ok_p (unsigned int regno, enum machine_mode mode)
>  	    && ST_REG_P (regno)
>  	    && (regno - ST_REG_FIRST) % 4 == 0);
>  
> +  if (mode == CCFmode)
> +    return (ISA_HAS_CCF
> +	    && FP_REG_P (regno));
> +
>    if (mode == CCmode)
>      return ISA_HAS_8CC ? ST_REG_P (regno) : regno == FPSW_REGNUM;

No need for the line split.

> @@ -11925,6 +12069,9 @@ mips_mode_ok_for_mov_fmt_p (enum machine_mode mode)
>  {
>    switch (mode)
>      {
> +    case CCFmode:
> +      return TARGET_HARD_FLOAT;
> +
>      case SFmode:
>        return TARGET_HARD_FLOAT;

Nit: might as well combine the cases.

> @@ -12190,6 +12337,10 @@ mips_secondary_reload_class (enum reg_class rclass,
>  	/* In this case we can use mov.fmt.  */
>  	return NO_REGS;
>  
> +      /* We don't need a reload if the pseudo is in memory in CCF mode.  */
> +      if (mode == CCFmode && regno == -1)
> +	return NO_REGS;
> +
>        /* Otherwise, we need to reload through an integer register.  */
>        return GR_REGS;
>      }

We should instead change the MEM_P in:

      if (MEM_P (x)
	  && (GET_MODE_SIZE (mode) == 4 || GET_MODE_SIZE (mode) == 8))
	/* In this case we can use lwc1, swc1, ldc1 or sdc1.  We'll use
	   pairs of lwc1s and swc1s if ldc1 and sdc1 are not supported.  */
	return NO_REGS;

to "(MEM_P (x) || regno == -1)".  There's not really anything special
about CCFmode here.

> @@ -15789,7 +15940,7 @@ mips_mult_zero_zero_cost (struct mips_sim *state, bool setting)
>  static void
>  mips_set_fast_mult_zero_zero_p (struct mips_sim *state)
>  {
> -  if (TARGET_MIPS16)
> +  if (TARGET_MIPS16 || !ISA_HAS_MULT)
>      /* No MTLO or MTHI available.  */
>      mips_tuning_info.fast_mult_zero_zero_p = true;
>    else

I think ISA_HAS_HILO would be better than ISA_HAS_MULT, and should
be used to define ISA_HAS_MULT, ISA_HAS_DMULT etc. in mips.h.
Same for a few other ISA_HAS_MULT tests in the patch.

Probably worth adding a comment saying that the choice doesn't
matter here since we will never need to move to HI or LO.

> @@ -17035,7 +17186,9 @@ mips_option_override (void)
>  
>    if ((target_flags_explicit & MASK_FLOAT64) != 0)
>      {
> -      if (TARGET_SINGLE_FLOAT && TARGET_FLOAT64)
> +      if (mips_isa_rev >= 6 && !TARGET_FLOAT64)
> +	error ("unsupported combination: %s", "-mips[32|64]r6 -mfp32");

Should use the actual arch rather than [32|64].  Note that ACONCAT ((...))
is available for building temporary strings.

> @@ -17052,8 +17205,12 @@ mips_option_override (void)
>    else
>      {
>        /* -msingle-float selects 32-bit float registers.  Otherwise the
> -	 float registers should be the same size as the integer ones.  */
> -      if (TARGET_64BIT && TARGET_DOUBLE_FLOAT)
> +	 float registers should be the same size as the integer ones.
> +	 MIPS R6 has 64-bit float registers regardless of the size of
> +	 the integer ones.  */
> +      if (mips_isa_rev >= 6 && TARGET_DOUBLE_FLOAT)
> +	target_flags |= MASK_FLOAT64;
> +      else if (TARGET_64BIT && TARGET_DOUBLE_FLOAT)
>  	target_flags |= MASK_FLOAT64;
>        else
>  	target_flags &= ~MASK_FLOAT64;

Sorry, a pet peeve is comments of the form "X must be Y" becoming
"X must be Y.  When foo, X might not be Y.".  Can you fold it into the
existing comment a bit more?  E.g.:

      /* -msingle-float selects 32-bit float registers.  On r6 and later,
	 -mdouble-float selects 64-bit float registers, since the old
	 paired-register model is not supported.  In other cases the float
	 registers should be the same size as the integer ones.  */
   
> @@ -17209,6 +17366,25 @@ mips_option_override (void)
>  	}
>      }
>  
> +  /* Set NaN and ABS defaults.  */
> +  if (mips_nan == MIPS_IEEE_754_DEFAULT && !ISA_HAS_NANLEGACY)
> +    mips_nan = MIPS_IEEE_754_2008;
> +  if (mips_abs == MIPS_IEEE_754_DEFAULT && !ISA_HAS_NANLEGACY)
> +    mips_abs = MIPS_IEEE_754_2008;
> +
> +  /* Check for NaN encoding support.  */
> +  if ((mips_nan == MIPS_IEEE_754_LEGACY
> +       || mips_abs == MIPS_IEEE_754_LEGACY)
> +      && !ISA_HAS_NANLEGACY)
> +    warning (0, "the %qs architecture does not support the NaN legacy"
> +	     " encoding", mips_arch_info->name);
> +
> +  if ((mips_nan == MIPS_IEEE_754_2008
> +       || mips_abs == MIPS_IEEE_754_2008)
> +      && !ISA_HAS_NAN2008)
> +    warning (0, "the %qs architecture does not support the NaN 2008"
> +	     " encoding", mips_arch_info->name);

The message is misleading.  mips_abs is about whether ABS is an arithmetic
or a sign-bit operation, not about the NaN encoding.

> @@ -17263,6 +17439,10 @@ mips_option_override (void)
>    if (TARGET_DSPR2)
>      TARGET_DSP = true;
>  
> +  if (TARGET_DSP && mips_isa_rev >= 6)
> +    error ("the %qs architecture does not support DSP instructions",
> +	   mips_arch_info->name);

I think we'd later ICE if the code went on to use DSP registers,
so better set TARGET_DSPR2 and TARGET_DSP to false here.

...ah, actually, I see you added conditions to ISA_HAS_DSP instead.
I think doing it here would be better.  The idea is that TARGET_*
should be accurate for the underlying architecture, with ISA_HAS_*
only varying depending on the ISA encoding.

> @@ -18006,7 +18195,10 @@ mips_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
>        trampoline[i++] = OP (MIPS_LOAD_PTR (STATIC_CHAIN_REGNUM,
>  					   static_chain_offset,
>  					   PIC_FUNCTION_ADDR_REGNUM));
> -      trampoline[i++] = OP (MIPS_JR (AT_REGNUM));
> +      if (ISA_HAS_JR)
> +	trampoline[i++] = OP (MIPS_JR (AT_REGNUM));
> +      else
> +	trampoline[i++] = OP (MIPS_JALR0 (AT_REGNUM));
>        trampoline[i++] = OP (MIPS_MOVE (PIC_FUNCTION_ADDR_REGNUM, AT_REGNUM));
>      }
>    else if (ptr_mode == DImode)
> @@ -18032,7 +18224,10 @@ mips_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
>        trampoline[i++] = OP (MIPS_LOAD_PTR (STATIC_CHAIN_REGNUM,
>  					   static_chain_offset - 12,
>  					   RETURN_ADDR_REGNUM));
> -      trampoline[i++] = OP (MIPS_JR (PIC_FUNCTION_ADDR_REGNUM));
> +      if (ISA_HAS_JR)
> +	trampoline[i++] = OP (MIPS_JR (PIC_FUNCTION_ADDR_REGNUM));
> +      else
> +	trampoline[i++] = OP (MIPS_JALR0 (PIC_FUNCTION_ADDR_REGNUM));
>        trampoline[i++] = OP (MIPS_MOVE (RETURN_ADDR_REGNUM, AT_REGNUM));
>      }
>    else
> @@ -18074,7 +18269,12 @@ mips_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
>  
>        /* Emit the JR here, if we can.  */
>        if (!ISA_HAS_LOAD_DELAY)
> -	trampoline[i++] = OP (MIPS_JR (PIC_FUNCTION_ADDR_REGNUM));
> +	{
> +	  if (ISA_HAS_JR)
> +	    trampoline[i++] = OP (MIPS_JR (PIC_FUNCTION_ADDR_REGNUM));
> +	  else
> +	    trampoline[i++] = OP (MIPS_JALR0 (PIC_FUNCTION_ADDR_REGNUM));
> +	}
>  
>        /* Emit the load of the static chain register.  */
>        opcode = OP (MIPS_LOAD_PTR (STATIC_CHAIN_REGNUM,
> @@ -18086,7 +18286,10 @@ mips_trampoline_init (rtx m_tramp, tree fndecl, rtx chain_value)
>        /* Emit the JR, if we couldn't above.  */
>        if (ISA_HAS_LOAD_DELAY)
>  	{
> -	  trampoline[i++] = OP (MIPS_JR (PIC_FUNCTION_ADDR_REGNUM));
> +	  if (ISA_HAS_JR)
> +	    trampoline[i++] = OP (MIPS_JR (PIC_FUNCTION_ADDR_REGNUM));
> +	  else
> +	    trampoline[i++] = OP (MIPS_JALR0 (PIC_FUNCTION_ADDR_REGNUM));
>  	  trampoline[i++] = OP (MIPS_NOP);
>  	}
>      }

Probably easier to make MIPS_JR return the JALR encoding for !ISA_HAS_JR.

> @@ -444,42 +446,12 @@ struct mips_cpu_info {
>  	  builtin_define ("__mips=4");					\
>  	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS4");			\
>  	}								\
> -      else if (ISA_MIPS32)						\
> +      else if (mips_isa >= 32 && mips_isa < 64)				\
>  	{								\
>  	  builtin_define ("__mips=32");					\
>  	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS32");		\
>  	}								\
> -      else if (ISA_MIPS32R2)						\
> -	{								\
> -	  builtin_define ("__mips=32");					\
> -	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS32");		\
> -	}								\
> -      else if (ISA_MIPS32R3)						\
> -	{								\
> -	  builtin_define ("__mips=32");					\
> -	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS32");		\
> -	}								\
> -      else if (ISA_MIPS32R5)						\
> -	{								\
> -	  builtin_define ("__mips=32");					\
> -	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS32");		\
> -	}								\
> -      else if (ISA_MIPS64)						\
> -	{								\
> -	  builtin_define ("__mips=64");					\
> -	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS64");		\
> -	}								\
> -      else if (ISA_MIPS64R2)						\
> -	{								\
> -	  builtin_define ("__mips=64");					\
> -	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS64");		\
> -	}								\
> -      else if (ISA_MIPS64R3)						\
> -	{								\
> -	  builtin_define ("__mips=64");					\
> -	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS64");		\
> -	}								\
> -      else if (ISA_MIPS64R5)						\
> +      else if (mips_isa >= 64)						\
>  	{								\
>  	  builtin_define ("__mips=64");					\
>  	  builtin_define ("_MIPS_ISA=_MIPS_ISA_MIPS64");		\

Oops, really should have noticed this one, sorry!  Thanks for cleaning it up.

> @@ -927,6 +937,9 @@ struct mips_cpu_info {
>  /* ISA has floating-point madd and msub instructions 'd = a * b [+-] c'.  */
>  #define ISA_HAS_FP_MADD4_MSUB4  ISA_HAS_FP4
>  
> +/* ISA has floating-point maddf and msubf instructions 'd = d [+-] a * b'.  */
> +#define ISA_HAS_FP_MADDF_MSUBF  (mips_isa_rev >= 6)
> +
>  /* ISA has floating-point madd and msub instructions 'c = a * b [+-] c'.  */
>  #define ISA_HAS_FP_MADD3_MSUB3  TARGET_LOONGSON_2EF

c = c ...

> +#define ISA_HAS_NANLEGACY	(mips_isa_rev <= 5)
> +
> +#define ISA_HAS_NAN2008		(mips_isa_rev >= 2)

Nit, but the existing enum has a "_" before the LEGACY and 2008.
Might as well have one here too for consistency.

> @@ -327,6 +329,7 @@ (define_attr "accum_in" "none,0,1,2,3,4,5" (const_string "none"))
>  ;; imadd	integer multiply-add
>  ;; idiv		integer divide 2 operands
>  ;; idiv3	integer divide 3 operands
> +;; idiv3nc	integer divide 3 operands without clobbering HI/LO
>  ;; move		integer register move ({,D}ADD{,U} with rt = 0)
>  ;; fmove	floating point register move
>  ;; fadd		floating point add/subtract

This type isn't used anywhere (which is good -- I think reusing idiv3
is the right thing to do).

> @@ -759,6 +762,11 @@ (define_mode_iterator MOVECC [SI (DI "TARGET_64BIT")
>  				   && !TARGET_LOONGSON_2EF
>  				   && !TARGET_MIPS5900")])
>  
> +;; This mode iterator allows :FPCC to be used anywhere that an FP condition
> +;; is needed.
> +(define_mode_iterator FPCC [(CC "!ISA_HAS_CCF")
> +			    (CCF "ISA_HAS_CCF")])
> +
>  ;; 32-bit integer moves for which we provide move patterns.
>  (define_mode_iterator IMOVE32
>    [SI
> @@ -848,7 +856,7 @@ (define_mode_attr si8_di5 [(SI "8") (DI "5")])
>  
>  ;; This attribute gives the best constraint to use for registers of
>  ;; a given mode.
> -(define_mode_attr reg [(SI "d") (DI "d") (CC "z")])
> +(define_mode_attr reg [(SI "d") (DI "d") (CC "z") (CCF "f")])
>  
>  ;; This attribute gives the format suffix for floating-point operations.
>  (define_mode_attr fmt [(SF "s") (DF "d") (V2SF "ps")])
> @@ -888,6 +896,9 @@ (define_mode_attr divide_condition
>  (define_mode_attr sqrt_condition
>    [(SF "!ISA_MIPS1") (DF "!ISA_MIPS1") (V2SF "TARGET_SB1")])
>  
> +;; This attribute provides the correct nmemonic for each FP condition mode.
> +(define_mode_attr fpcmp [(CC "c") (CCF "cmp")])
> +
>  ;; This code iterator allows signed and unsigned widening multiplications
>  ;; to use the same template.
>  (define_code_iterator any_extend [sign_extend zero_extend])

This seems really clean, thanks.

> @@ -1105,16 +1126,25 @@ (define_expand "ctrap<mode>4"
>  			    [(match_operand:GPR 1 "reg_or_0_operand")
>  			     (match_operand:GPR 2 "arith_operand")])
>  	    (match_operand 3 "const_0_operand"))]
> -  "ISA_HAS_COND_TRAP"
> +  "ISA_HAS_COND_TRAPI || ISA_HAS_COND_TRAP"
>  {
>    mips_expand_conditional_trap (operands[0]);
>    DONE;
>  })
>  
> +(define_insn "*conditional_trapi<mode>"
> +  [(trap_if (match_operator:GPR 0 "trap_comparison_operator"
> +			[(match_operand:GPR 1 "reg_or_0_operand" "dJ")
> +			 (match_operand:GPR 2 "const_arith_operand" "I")])
> +	    (const_int 0))]
> +  "ISA_HAS_COND_TRAPI"
> +  "t%C0\t%z1,%2"
> +  [(set_attr "type" "trap")])
> +
>  (define_insn "*conditional_trap<mode>"
>    [(trap_if (match_operator:GPR 0 "trap_comparison_operator"
>  				[(match_operand:GPR 1 "reg_or_0_operand" "dJ")
> -				 (match_operand:GPR 2 "arith_operand" "dI")])
> +				 (match_operand:GPR 2 "register_operand" "d")])
>  	    (const_int 0))]
>    "ISA_HAS_COND_TRAP"
>    "t%C0\t%z1,%2"

It's better to keep the "d" and "I" alternatives as part of one pattern,
since that gives the register allocator more freedom.  So I think we want
to keep *conditional_trap<mode> as-is and add a new pattern for
ISA_HAS_COND_TRAP && !ISA_HAS_COND_TRAPI.

In commutative comparisons the zero should come second, so it'd be
better to use reg_or_0_operand/dJ instead of register_operand/d.
Same goes for the register_operand in mips_expand_conditional_trap.

> @@ -1484,13 +1514,13 @@ (define_expand "mul<mode>3"
>    [(set (match_operand:GPR 0 "register_operand")
>  	(mult:GPR (match_operand:GPR 1 "register_operand")
>  		  (match_operand:GPR 2 "register_operand")))]
> -  "ISA_HAS_<D>MULT"
> +  "ISA_HAS_<D>MULT || ISA_HAS_R6<D>MUL"
>  {
>    rtx lo;
>  
> -  if (TARGET_LOONGSON_2EF || TARGET_LOONGSON_3A)
> -    emit_insn (gen_mul<mode>3_mul3_loongson (operands[0], operands[1],
> -                                             operands[2]));
> +  if (TARGET_LOONGSON_2EF || TARGET_LOONGSON_3A || ISA_HAS_R6<D>MUL)
> +    emit_insn (gen_mul<mode>3_mul3_r6 (operands[0], operands[1],
> +                                       operands[2]));
>    else if (ISA_HAS_<D>MUL3)
>      emit_insn (gen_mul<mode>3_mul3 (operands[0], operands[1], operands[2]));
>    else if (TARGET_MIPS16)

It's a bit confusing to use "_r6" as the suffix for a pattern that's
shared with Loongson.  Maybe "_nohilo", or something?

> @@ -3872,7 +4018,7 @@ (define_expand "extvmisalign<mode>"
>  	(sign_extract:GPR (match_operand:BLK 1 "memory_operand")
>  			  (match_operand 2 "const_int_operand")
>  			  (match_operand 3 "const_int_operand")))]
> -  "!TARGET_MIPS16"
> +  "ISA_HAS_LWL_LWR && !TARGET_MIPS16"

Please put the TARGET_MIPS16 condition in the ISA_HAS_LWL_LWR definition.

> @@ -6906,6 +7059,41 @@ (define_insn "*mov<SCALARF:mode>_on_<MOVECC:mode>"
>    [(set_attr "type" "condmove")
>     (set_attr "mode" "<SCALARF:MODE>")])
>  
> +(define_insn "*sel<code><GPR:mode>_using_<GPR2:mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=d,d")
> +	(if_then_else:GPR
> +	 (equality_op:GPR2 (match_operand:GPR2 1 "register_operand" "d,d")
> +		           (const_int 0))
> +	 (match_operand:GPR 2 "reg_or_0_operand" "d,J")
> +	 (match_operand:GPR 3 "reg_or_0_operand" "J,d")))]
> +  "ISA_HAS_SEL
> +   && (register_operand (operands[2], <GPR:MODE>mode)
> +       ^ register_operand (operands[3], <GPR:MODE>mode))"
> +  "@
> +   <sel>\t%0,%2,%1
> +   <selinv>\t%0,%3,%1"
> +  [(set_attr "type" "condmove")
> +   (set_attr "mode" "<GPR:MODE>")])

"!=" seems more obvious than "^".

> @@ -6914,8 +7102,13 @@ (define_expand "mov<mode>cc"
>  	(if_then_else:GPR (match_dup 5)
>  			  (match_operand:GPR 2 "reg_or_0_operand")
>  			  (match_operand:GPR 3 "reg_or_0_operand")))]
> -  "ISA_HAS_CONDMOVE"
> +  "ISA_HAS_CONDMOVE || ISA_HAS_SEL"
>  {
> +  if (ISA_HAS_SEL
> +      && (GET_MODE (XEXP (operands[1], 0)) != SImode)
> +      && (GET_MODE (XEXP (operands[1], 0)) != DImode))
> +    FAIL;
> +
>    mips_expand_conditional_move (operands);
>    DONE;
>  })

Maybe INTEGRAL_MODE_P here?

> @@ -6924,10 +7117,16 @@ (define_expand "mov<mode>cc"
>    [(set (match_dup 4) (match_operand 1 "comparison_operator"))
>     (set (match_operand:SCALARF 0 "register_operand")
>  	(if_then_else:SCALARF (match_dup 5)
> -			      (match_operand:SCALARF 2 "register_operand")
> -			      (match_operand:SCALARF 3 "register_operand")))]
> -  "ISA_HAS_FP_CONDMOVE"
> -{
> +			      (match_operand:SCALARF 2 "reg_or_0_operand")
> +			      (match_operand:SCALARF 3 "reg_or_0_operand")))]
> +  "ISA_HAS_FP_CONDMOVE
> +   || (ISA_HAS_SEL && ISA_HAS_CCF)"
> +{
> +  if (ISA_HAS_SEL
> +      && GET_MODE (XEXP (operands[1], 0)) != SFmode
> +      && GET_MODE (XEXP (operands[1], 0)) != DFmode)
> +    FAIL;
> +
>    mips_expand_conditional_move (operands);
>    DONE;
>  })

Same for FLOAT_MODE_P here.

Looks really good though.  I was afraid r6 was going to be more invasive
than it ended up being.

Thanks,
Richard


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