Robert Suchanek <robert.sucha...@imgtec.com> writes: > The patch primarily fixes an ICE with out-of-range values to the > __builtin_msa_insve* intrinsics. > > The compiler segfaults in mips_legitimize_const_move () as it tries to > split symbol that has NULL_RTX value and gets here because the patterns > reject the operand and a new move for the constant is being introduced. > > The INSVE.DF instruction cannot use register based access to the > element, thus, the attempt is obviously wrong and I think that we should > be catching this invalid input early.
Agreed, catching problems with arguments to builtins early sounds better to me generally. > I took the opportunity to check all the builtins with literal integers > except those that use full range of a type as truncation warnings are > generated. The diagnostics is slightly more meaningful and the valid > ranges align with the documentation. Some comments on the implementation below. Just some further cleanup. > gcc/ > * config/mips/mips.c (mips_expand_builtin_insn): Check input ranges > of literal integer arguments. > > gcc/testsuite/ > > * gcc.target/mips/msa-builtins-err.c: New test. > --- > gcc/config/mips/mips.c | 56 +++++- > gcc/testsuite/gcc.target/mips/msa-builtins-err.c | 241 > +++++++++++++++++++++++ > 2 files changed, 289 insertions(+), 8 deletions(-) create mode 100644 > gcc/testsuite/gcc.target/mips/msa-builtins-err.c > > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index > 44cdeb7..ddb64fb 100644 > --- a/gcc/config/mips/mips.c > +++ b/gcc/config/mips/mips.c > @@ -16542,6 +16542,7 @@ mips_expand_builtin_insn (enum insn_code icode, > unsigned int nops, > struct expand_operand *ops, bool has_target_p) { > machine_mode imode; > + HOST_WIDE_INT arglo, arghi, argno = -1; arglo/arghi should really be initialised to zero as the control flow is complex that ensures they are set when argno != -1. Perhaps rangelo and rangehi to distinguish from the operand numbers. argno (perhaps error_opno) can be an int. > > switch (icode) > { > @@ -16570,11 +16571,18 @@ mips_expand_builtin_insn (enum insn_code > icode, unsigned int nops, > case CODE_FOR_msa_subvi_w: > case CODE_FOR_msa_subvi_d: > gcc_assert (has_target_p && nops == 3); > + arglo = 0; > + arghi = 31; > /* We only generate a vector of constants iff the second argument > is an immediate. We also validate the range of the immediate. */ > - if (!CONST_INT_P (ops[2].value) > - || !IN_RANGE (INTVAL (ops[2].value), 0, 31)) > + if (!CONST_INT_P (ops[2].value)) > break; > + if (CONST_INT_P (ops[2].value) > + && !IN_RANGE (INTVAL (ops[2].value), arglo, arghi)) > + { > + argno = 2; > + break; > + } The error should really be shown if a !CONST_INT_P is found too. I.e. if (!CONST_INT_P (ops[2].value) || !IN_RANGE (INTVAL (ops[2].value), 0, 31)) { error_opno = 2; break; } Similarly throughout. > ops[2].mode = ops[0].mode; > ops[2].value = mips_gen_const_int_vector (ops[2].mode, > INTVAL (ops[2].value)); > @@ -16601,11 +16609,18 @@ mips_expand_builtin_insn (enum insn_code > icode, unsigned int nops, > case CODE_FOR_msa_mini_s_w: > case CODE_FOR_msa_mini_s_d: > gcc_assert (has_target_p && nops == 3); > + arglo = -16; > + arghi = 15; > /* We only generate a vector of constants iff the second argument > is an immediate. We also validate the range of the immediate. */ > - if (!CONST_INT_P (ops[2].value) > - || !IN_RANGE (INTVAL (ops[2].value), -16, 15)) > + if (!CONST_INT_P (ops[2].value)) > break; > + if (CONST_INT_P (ops[2].value) > + && !IN_RANGE (INTVAL (ops[2].value), arglo, arghi)) > + { > + argno = 2; > + break; > + } > ops[2].mode = ops[0].mode; > ops[2].value = mips_gen_const_int_vector (ops[2].mode, > INTVAL (ops[2].value)); > @@ -16688,10 +16703,16 @@ mips_expand_builtin_insn (enum insn_code > icode, unsigned int nops, > case CODE_FOR_msa_srli_w: > case CODE_FOR_msa_srli_d: > gcc_assert (has_target_p && nops == 3); > - if (!CONST_INT_P (ops[2].value) > - || !IN_RANGE (INTVAL (ops[2].value), 0, > - GET_MODE_UNIT_PRECISION (ops[0].mode) - 1)) > + if (!CONST_INT_P (ops[2].value)) > break; > + arglo = 0; > + arghi = GET_MODE_UNIT_BITSIZE (ops[0].mode) - 1; > + if (CONST_INT_P (ops[2].value) > + && !IN_RANGE (INTVAL (ops[2].value), arglo, arghi)) > + { > + argno = 2; > + break; > + } > ops[2].mode = ops[0].mode; > ops[2].value = mips_gen_const_int_vector (ops[2].mode, > INTVAL (ops[2].value)); > @@ -16710,6 +16731,14 @@ mips_expand_builtin_insn (enum insn_code icode, > unsigned int nops, > imode = GET_MODE_INNER (ops[0].mode); > ops[1].value = lowpart_subreg (imode, ops[1].value, ops[1].mode); > ops[1].mode = imode; > + arglo = 0; > + arghi = GET_MODE_NUNITS (ops[0].mode) - 1; > + if (!CONST_INT_P (ops[3].value) > + || !IN_RANGE (INTVAL (ops[3].value), arglo, arghi)) > + { > + argno = 2; > + break; > + } > ops[3].value = GEN_INT (1 << INTVAL (ops[3].value)); > break; > > @@ -16722,6 +16751,14 @@ mips_expand_builtin_insn (enum insn_code icode, > unsigned int nops, > gcc_assert (has_target_p && nops == 4); > std::swap (ops[1], ops[2]); > std::swap (ops[1], ops[3]); > + arglo = 0; > + arghi = GET_MODE_NUNITS (ops[0].mode) - 1; > + if (!CONST_INT_P (ops[3].value) > + || !IN_RANGE (INTVAL (ops[3].value), arglo, arghi)) > + { > + argno = 2; > + break; > + } > ops[3].value = GEN_INT (1 << INTVAL (ops[3].value)); > break; > > @@ -16746,7 +16783,10 @@ mips_expand_builtin_insn (enum insn_code icode, > unsigned int nops, > break; > } > > - if (!maybe_expand_insn (icode, nops, ops)) > + if (argno != -1) > + error ("%s argument to the built-in must be in range %ld to %ld", > + argno == 2 ? "second" : "third", arglo, arghi); I'd reword this to avoid the 2/3 etc and mention the need for a constant given it will appear when a variable is used now as well. error ("argument %d to the built-in must be a constant in range %ld to %ld", error_opno, rangelo, rangehi); Given we go to the effort of faking a return when the instruction fails to expand then we should do the same here: return has_target_p ? gen_reg_rtx (ops[0].mode) : const0_rtx; Can you repost with those changes for quick 2nd review? Thanks, Matthew