Hi Matthew,

Revised patch attached.

Tested with mips-img-linux-gnu and bootstrapped x86_64-unknown-linux-gnu.

> > 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.

gen_int_mode cannot be used to generate a vector of constants as it expects a scalar mode.
AFAICS, there isn't any generic helper to replace this.

> 
> > mips_const_vector_same_bytes_p
> comment on this function is same as previous function

Corrected.

> 
> > mips_msa_idiv_insns
> Why not just update mips_idiv_insns and add a mode argument?

Done. 

> 
> > Implement TARGET_PRINT_OPERAND.
> Comment spacing between 'E' 'B' and description is different to existing

Updated.

> 
> > mips_print_operand
> case 'v' subcases V4SImode and V4SFmode are identical. same for DI/DF.

Updated.

> 
> >@@ -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.

Indeed.  Removed.

> 
> >@@ -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.

Done.

> 
> >@@ -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.

Fixed.

> 
> +   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
> 

The changes have been reverted because of link to MAX_FIXED_MODE_SIZE macro
causing failures in the by_pieces logic if MOVE_MAX_PIECES is larger than MAX_FIXED_MODE_SIZE.
As it stands, vector modes appear to be handled explicitly in the common code
so it's unlikely we need to modify any of these.
If they do then it will be included in the follow up.

> > 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)

Will do.

> 
> >+    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.

That's correct. I've moved it to mips_gen_cost_int_vector and used gen_int_mode.

> 
> >@@ -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?

I'm not sure but it appears to be introduced long ago.
I removed it and used only mips_idiv_insns with the mode.

> 
> >@@ -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.

Reverted.

> 
> >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.

Added.

> 
> >+(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.

Agreed. 

> 
> > "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.

Done.

> 
> 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.

Will do.

Regards,
Robert