Re: [PATCH, rs6000] Fix PR80376 (somewhat)

[Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>]

Hi Segher,

> On Apr 10, 2017, at 4:31 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> On Mon, Apr 10, 2017 at 12:08:50PM -0500, Bill Schmidt wrote:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80376 shows an ICE on invalid
>> code when using certain flavors of the vec_xxpermdi intrinsic.  The root
>> cause is that we were not checking the parameter for a legal value, and an
>> illegal value was being provided by the user (only an integer constant is
>> permissible for argument 3).
>> 
>> However, this brings up a slightly larger problem, in that the invalid
>> vec_xxpermdi call is nested within another call.  We have a lot of cases
>> in rs6000.c where we signal an error message and replace the offending
>> construct with a const0_rtx.  In this case, that const0_rtx was being used
>> as a vector argument to another call, leading to a follow-up ICE that is
>> not easy to parse.
> 
> i386 does this:
> 
> /* Errors in the source file can cause expand_expr to return const0_rtx
>   where we expect a vector.  To avoid crashing, use one of the vector
>   clear instructions.  */
> static rtx
> safe_vector_operand (rtx x, machine_mode mode)
> {
>  if (x == const0_rtx)
>    x = CONST0_RTX (mode);
>  return x;
> }
> 
> used in e.g. ix86_expand_binop_builtin as
>  if (VECTOR_MODE_P (mode0))
>    op0 = safe_vector_operand (op0, mode0);
> We might want something similar :-)

That's a nice idea.  That would at least keep us from having to fix this in 2-3 dozen places.
Let me take a look.

Thanks!
Bill

> 
>> Index: gcc/config/rs6000/rs6000.c
>> ===================================================================
>> --- gcc/config/rs6000/rs6000.c	(revision 246804)
>> +++ gcc/config/rs6000/rs6000.c	(working copy)
>> @@ -15586,6 +15586,11 @@ rs6000_expand_ternop_builtin (enum insn_code icode
>>            || icode == CODE_FOR_vsx_xxpermdi_v2di
>>            || icode == CODE_FOR_vsx_xxpermdi_v2df_be
>>            || icode == CODE_FOR_vsx_xxpermdi_v2di_be
>> +	   || icode == CODE_FOR_vsx_xxpermdi_v1ti
>> +	   || icode == CODE_FOR_vsx_xxpermdi_v4sf
>> +	   || icode == CODE_FOR_vsx_xxpermdi_v4si
>> +	   || icode == CODE_FOR_vsx_xxpermdi_v8hi
>> +	   || icode == CODE_FOR_vsx_xxpermdi_v16qi
>>            || icode == CODE_FOR_vsx_xxsldwi_v16qi
>>            || icode == CODE_FOR_vsx_xxsldwi_v8hi
>>            || icode == CODE_FOR_vsx_xxsldwi_v4si
> 
> The existing code is indented with spaces only; please keep the style
> consistent (fixing it is fine, but then the whole block or function).

OK.

> 
>> --- gcc/config/rs6000/vector.md	(revision 246804)
>> +++ gcc/config/rs6000/vector.md	(working copy)
>> @@ -109,6 +109,11 @@
>>     {
>>       if (CONSTANT_P (operands[1]))
>> 	{
>> +	  /* Handle cascading error conditions.  */
>> +	  if (VECTOR_MODE_P (<MODE>mode)
>> +	      && !VECTOR_MODE_P (GET_MODE (operands[1])))
>> +	    internal_error ("non-vector constant found where vector expected");
>> +
>> 	  if (FLOAT128_VECTOR_P (<MODE>mode))
>> 	    {
>> 	      if (!easy_fp_constant (operands[1], <MODE>mode))
> 
> You might not need this with the suggestion above?
> 
> The patch is okay if you want that for now; it's an improvement over
> what we have.
> 
> 
> Segher
>