Hi Robert,
Robert Suchanek <robert.sucha...@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
