This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [PATCH] MIPS MSA: Fix ICE when using out-of-range values to intrinsics
- From: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- To: Robert Suchanek <Robert dot Suchanek at imgtec dot com>, "Catherine_Moore at mentor dot com" <Catherine_Moore at mentor dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 28 Nov 2016 16:17:33 +0000
- Subject: RE: [PATCH] MIPS MSA: Fix ICE when using out-of-range values to intrinsics
- Authentication-results: sourceware.org; auth=none
- References: <B5E67142681B53468FAF6B7C31356562519C72FF@HHMAIL01.hh.imgtec.org>
Robert Suchanek <Robert.Suchanek@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