This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH] MIPS MSA: Fix ICE when using out-of-range values to intrinsics


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]