RE: [PATCH 1/4] [MIPS] Add support for MIPS SIMD Architecture (MSA)

[Matthew Fortune <Matthew dot Fortune at imgtec dot com>]

Hi Robert,

Robert Suchanek <Robert.Suchanek@imgtec.com> writes:
> Revised patch attached.
> 
> Tested with mips-img-linux-gnu and bootstrapped x86_64-unknown-linux-
> gnu.

One small tweak, ChangeLog should wrap at 74 columns. Please consider the
full list of authors for this patch as it has had many major contributors
now. I believe this includes at least the following for the implementation
but fewer for the testsuite updates:

Robert Suchanek
Sameera Deshpande
Matthew Fortune
Graham Stott
Chao-ying Fu

Otherwise, OK to commit!

Matthew

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