Hi Robert,
Apologies for the long delay again. This patch is hard to get through. My
comments
are not all in source sequence but I've tried to keep them short. With a few
minor
things fixed and some trivial style issues done then this is ready to go. I've
left
a number of things to look at after getting this patch in as I can't track any
more
significant updates to this:
> mips_gen_const_int_vector
This should use gen_int_for_mode instead of GEN_INT to avoid the issues that
msa_ldi is
trying to handle.
> mips_const_vector_same_bytes_p
comment on this function is same as previous function
> mips_msa_idiv_insns
Why not just update mips_idiv_insns and add a mode argument?
> Implement TARGET_PRINT_OPERAND.
Comment spacing between 'E' 'B' and description is different to existing
> mips_print_operand
case 'v' subcases V4SImode and V4SFmode are identical. same for DI/DF.
>@@ -12272,13 +12837,25 @@ mips_class_max_nregs (enum reg_class rclass,
>machine_mode mode)
> if (hard_reg_set_intersect_p (left, reg_class_contents[(int) ST_REGS]))
> {
> if (HARD_REGNO_MODE_OK (ST_REG_FIRST, mode))
>- size = MIN (size, 4);
>+ {
>+ if (MSA_SUPPORTED_MODE_P (mode))
>+ size = MIN (size, UNITS_PER_MSA_REG);
>+ else
>+ size = MIN (size, UNITS_PER_FPREG);
>+ }
>+
This hunk should be removed. MSA modes are not supported in ST_REGS.
>@@ -12299,6 +12876,10 @@ mips_cannot_change_mode_class (machine_mode from,
> && INTEGRAL_MODE_P (from) && INTEGRAL_MODE_P (to))
> return false;
>
>+ /* Allow conversions between different MSA vector modes and TImode. */
Remove 'and TImode' we do not support it.
>@@ -19497,9 +21284,64 @@ mips_expand_vec_unpack (rtx operands[2], bool
>unsigned_p, bool high_p)
>+ if (!unsigned_p)
>+ {
>+ /* Extract sign extention for each element comparing each element with
>+ immediate zero. */
>+ tmp = gen_reg_rtx (imode);
>+ emit_insn (cmpFunc (tmp, operands[1], CONST0_RTX (imode)));
>+ }
>+ else
>+ {
>+ tmp = force_reg (imode, CONST0_RTX (imode));
>+ }
Indentation and unnecessary braces on the else.
+ A single N-word move is usually the same cost as N single-word moves.
+ For MSA, we set MOVE_MAX to 16 bytes.
+ Then, MAX_MOVE_MAX is 16 unconditionally. */
+#define MOVE_MAX (TARGET_MSA ? 16 : UNITS_PER_WORD)
+#define MAX_MOVE_MAX 16
The 16 here should be UNITS_PER_MSA_REG
> mips_expand_builtin_insn
General comment about operations that take an immediate. There is code to
perform range
checking but it does not seem to leave any trail when the maybe_expand_insn
fails to
tell the user it was an out of range immediate that was the problem. (follow up
work)
>+ case CODE_FOR_msa_andi_b:
>+ case CODE_FOR_msa_ori_b:
>+ case CODE_FOR_msa_nori_b:
>+ case CODE_FOR_msa_xori_b:
>+ gcc_assert (has_target_p && nops == 3);
>+ if (!CONST_INT_P (ops[2].value))
>+ break;
>+ ops[2].mode = ops[0].mode;
>+ /* We need to convert the unsigned value to signed. */
>+ val = sext_hwi (INTVAL (ops[2].value),
>+ GET_MODE_UNIT_PRECISION (ops[2].mode));
>+ ops[2].value = mips_gen_const_int_vector (ops[2].mode, val);
>+ break
Isn't the sext_hwi just effectively doing what gen_int_for_mode would? Fixing
mips_gen_const_int_vector would eliminate all of them.
>@@ -527,7 +551,9 @@ (define_attr "insn_count" ""
> (const_int 2)
>
> (eq_attr "type" "idiv,idiv3")
>- (symbol_ref "mips_idiv_insns ()")
>+ (cond [(eq_attr "mode" "TI")
>+ (symbol_ref "mips_msa_idiv_insns () * 4")]
>+ (symbol_ref "mips_idiv_insns () * 4"))
Why *4?
>@@ -1537,8 +1553,10 @@ FP_ASM_SPEC "\
> #define LONG_LONG_ACCUM_TYPE_SIZE (TARGET_64BIT ? 128 : 64)
>
> /* long double is not a fixed mode, but the idea is that, if we
>- support long double, we also want a 128-bit integer type. */
>-#define MAX_FIXED_MODE_SIZE LONG_DOUBLE_TYPE_SIZE
>+ support long double, we also want a 128-bit integer type.
>+ For MSA, we support an integer type with a width of BITS_PER_MSA_REG. */
>+#define MAX_FIXED_MODE_SIZE \
>+ (TARGET_MSA ? BITS_PER_MSA_REG : LONG_DOUBLE_TYPE_SIZE)
This doesn't seem right. We don't support TImode with MSA.
>diff --git a/gcc/config/mips/mips-msa.md b/gcc/config/mips/mips-msa.md
>new file mode 100644
>index 0000000..79e382d
>--- /dev/null
>+++ b/gcc/config/mips/mips-msa.md
>@@ -0,0 +1,2725 @@
>+(define_insn "msa_copy_s_<msafmt_f>"
>+ [(set (match_operand:<UNITMODE> 0 "register_operand" "=d")
>+ (vec_select:<UNITMODE>
>+ (match_operand:MSA_W 1 "register_operand" "f")
>+ (parallel [(match_operand 2 "const_<indeximm>_operand" "")])))]
>+ "ISA_HAS_MSA"
>+ "copy_s.<msafmt>\t%0,%w1[%2]"
>+ [(set_attr "type" "simd_copy")
>+ (set_attr "mode" "<MODE>")])
There is a sign extend version of this pattern needed for TARGET_64BIT widening
to DImode.
>+(define_expand "msa_ldi<mode>"
>+ [(match_operand:IMSA 0 "register_operand")
>+ (match_operand 1 "const_imm10_operand")]
>+ "ISA_HAS_MSA"
>+{
>+ if (<MODE>mode == V16QImode)
>+ operands[1] = GEN_INT (trunc_int_for_mode (INTVAL (operands[1]),
>+ <UNITMODE>mode));
I still don't like this expander. I think it needs moving to the builtin
expansion code as a follow up.
> "msa_fill_<msafmt_f>"
The fill with constant could be extended to handle all immediates for LDI
including
those for which the constant is wider that 8-bit but contains a replicated
value that
a narrower LDI could create. (Just for follow up work)
General comment: A number of TARGET_MSA instances should be ISA_HAS_MSA please
check.
I'm not sure but I don't think the mapping from FP comparisons to signalling vs
quiet
compares is correct. It needs checking in detail for a follow up.
Thanks,
Matthew
