Richard Sandiford <rdsandif...@googlemail.com> writes:
> Sorry for the slow review.
And my slow response :-)
> Matthew Fortune <matthew.fort...@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?
You have actually covered the cases I was concerned about below. The
problem cases are those tests that already have a isa/isa_rev >= ...
> 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.
OK. Steve has actually been working on this in parallel to the
review and has taken this approach.
> 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.
There appear to be about 9 tests that are not fixed by educating mips.exp
about flags which are not supported on R6. Steve has initially dealt with
these via forbid_cpu=mips.*r6 but I guess it would be cleaner to try and
support an isa_rev range. I'll see we can club together enough tcl skills
to write it :-)
> 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.
Thanks. I've omitted the detail about what the cases are as the code
speaks for itself.
> > (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.
It seems I cannot use cond in a predicate expression, so I've had to
leave it as is.
> > 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.
I agree this could be simpler and your comment has made me realise the
implementation in the patch is wrong for configurations like
mipsisa32r6-unknown-linux-gnu. The issue for both the current patch and
your suggestion is that they rely on MIPS_ISA_LEVEL_SPEC having been
applied but this only happens in the vendor triplets. The --with-arch*
options used with mips-unknown-linux-gnu would be fine as they place
an arch option on the command line.
If I add MIPS_ISA_LEVEL_SPEC to the DRIVER_SELF_SPECS generic
definition in mips.h then I believe that would fix the problem. Any new
spec I add for R6/nan setting would also need adding to the generic
DRIVER_SELF_SPECS in mips.h and any vendor definitions of
DRIVER_SELF_SPECS.
> > 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
> > @@ -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 :-/
Indeed! It follows similar code in other backends but is hideous. I'm not
sure changing it would improve matters though as we would end up with lots
of nested if statements with temporaries for each level of the check. I
have however added a comment to state the pattern being matched.
> > @@ -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.
OK. Am I right in thinking that all MODE_CC modes are considered to be
4-byte?
> > @@ -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.
Done. I don't think I had fully realised that this code is entirely
irrelevant to r6 given there are no accumulators which removes the
need to ever zero them.
> > @@ -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.
Changed to:
error ("the %qs architecture does not support %<-mfp32%>",
mips_arch_info->name);
> > @@ -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.
Thanks, this is/was ugly. I've changed ISA_HAS_NAN2008 to
ISA_HAS_IEEE_754_2008 and made the error reporting into this:
if ((mips_nan == MIPS_IEEE_754_2008
|| mips_abs == MIPS_IEEE_754_2008)
&& !ISA_HAS_IEEE_754_2008)
warning (0, "the %qs architecture does not support %<-m%s=2008%>"
mips_arch_info->name,
mips_nan == MIPS_IEEE_754_2008 ? "nan" : "abs");
Is that style OK?
> > @@ -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.
OK. While working on this code I did change my mind a few times as I
believe there are some uses of TARGET_DSP that should actually be
ISA_HAS_DSP. I started to change them but wasn't sure.
> > @@ -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.
As above these now reflect the IEEE 754 support rather than specifically
nan support. Underscores now match the other IEEE 754 enums/macros.
> > @@ -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.
OK. I think I've followed that and changed appropriately.
> It's a bit confusing to use "_r6" as the suffix for a pattern that's
> shared with Loongson. Maybe "_nohilo", or something?
Seems reasonable.
> Looks really good though. I was afraid r6 was going to be more invasive
> than it ended up being.
I suspect in time the optimisations to tune for and use new features of R6
will be more invasive but initial work has turned out quite clean.
I'll post the patch with fixes in place once we have discussed the specs
issue which I think is the only one that needs further debate.
Thanks,
Matthew
