Sorry for the slow review.
Matthew Fortune <[email protected]> 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
