Re: [PATCH] Fix ICE with V1DImode ctor (PR middle-end/71626)

[Richard Biener <rguenther at suse dot de>]

On Mon, 27 Jun 2016, Jakub Jelinek wrote:

> Hi!
> 
> This patch is an attempt to fix ICE on the following testcase.
> In output_constant_pool_2 we assume that for vector modes, force_const_mem
> must be a CONST_VECTOR, but for the weirdo vector modes with single element
> it could very well be just a SUBREG of some other constant.
> This isn't enough though, e.g. for -fpic we shouldn't force it into constant
> pool constant, because it needs relocation.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2016-06-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/71626
> 	* output.h (compute_reloc_for_rtx): New prototype.
> 	* varasm.c (output_constant_pool_2): Handle SUBREGs for V1??mode
> 	vectors.
> 	(compute_reloc_for_rtx): No longer static.
> 	* config/i386/i386.c (ix86_expand_vector_move): For V1??mode SUBREG
> 	of scalar constant that needs relocation, force the constant into
> 	the inner mode reg first.
> 
> 	* gcc.c-torture/execute/pr71626-1.c: New test.
> 	* gcc.c-torture/execute/pr71626-2.c: New test.
> 
> --- gcc/output.h.jj	2016-01-04 14:55:53.000000000 +0100
> +++ gcc/output.h	2016-06-27 12:21:41.529484513 +0200
> @@ -349,6 +349,9 @@ extern bool decl_readonly_section (const
>     given a constant expression.  */
>  extern int compute_reloc_for_constant (tree);
>  
> +/* Similarly, but for RTX.  */
> +extern int compute_reloc_for_rtx (const_rtx);
> +
>  /* User label prefix in effect for this compilation.  */
>  extern const char *user_label_prefix;
>  
> --- gcc/varasm.c.jj	2016-06-10 20:24:03.000000000 +0200
> +++ gcc/varasm.c	2016-06-27 12:21:36.062553957 +0200
> @@ -3834,6 +3834,17 @@ output_constant_pool_2 (machine_mode mod
>          machine_mode submode = GET_MODE_INNER (mode);
>  	unsigned int subalign = MIN (align, GET_MODE_BITSIZE (submode));
>  
> +	/* For V1??mode, allow SUBREGs.  */
> +	if (GET_CODE (x) == SUBREG
> +	    && GET_MODE_NUNITS (mode) == 1
> +	    && GET_MODE_BITSIZE (submode) == GET_MODE_BITSIZE (mode)
> +	    && SUBREG_BYTE (x) == 0
> +	    && GET_MODE (SUBREG_REG (x)) == submode)
> +	  {
> +	    output_constant_pool_2 (submode, SUBREG_REG (x), align);
> +	    break;
> +	  }
> +

I wonder why we can rely on the MODE_FLOAT case not seeing
(subreg:DF (reg:V2DF ) 0) but have to handle
(subreg:V1DF (reg:DF ...) 0) in the MODE_VECTOR case.

I do remember trying to optimize constant pool handling to not
re-emit DFmode constants if we have a VxDFmode constant with
a matching component.

That said, if we don't allow arbitrary subregs maybe we should
adjust the constant_descriptor_rtx management code to assert
that only the above form of subregs can appear?

Richard.

>  	gcc_assert (GET_CODE (x) == CONST_VECTOR);
>  	units = CONST_VECTOR_NUNITS (x);
>  
> @@ -6637,7 +6648,7 @@ compute_reloc_for_rtx_1 (const_rtx x)
>     is a mask for which bit 1 indicates a global relocation, and bit 0
>     indicates a local relocation.  */
>  
> -static int
> +int
>  compute_reloc_for_rtx (const_rtx x)
>  {
>    switch (GET_CODE (x))
> --- gcc/config/i386/i386.c.jj	2016-06-24 12:59:29.000000000 +0200
> +++ gcc/config/i386/i386.c	2016-06-27 12:23:15.504290801 +0200
> @@ -19605,6 +19605,20 @@ ix86_expand_vector_move (machine_mode mo
>    if (push_operand (op0, VOIDmode))
>      op0 = emit_move_resolve_push (mode, op0);
>  
> +  /* For V1XXmode subregs of scalar constants, force the scalar into
> +     reg first if it would need relocations.  */
> +  if (SUBREG_P (op1)
> +      && CONSTANT_P (SUBREG_REG (op1))
> +      && VECTOR_MODE_P (mode)
> +      && GET_MODE_NUNITS (mode) == 1
> +      && GET_MODE (SUBREG_REG (op1)) == GET_MODE_INNER (mode)
> +      && compute_reloc_for_rtx (SUBREG_REG (op1)))
> +    {
> +      rtx r = force_reg (GET_MODE (SUBREG_REG (op1)), SUBREG_REG (op1));
> +      op1 = simplify_gen_subreg (mode, r, GET_MODE (SUBREG_REG (op1)),
> +				 SUBREG_BYTE (op1));
> +    }
> +
>    /* Force constants other than zero into memory.  We do not know how
>       the instructions used to build constants modify the upper 64 bits
>       of the register, once we have that information we may be able
> --- gcc/testsuite/gcc.c-torture/execute/pr71626-1.c.jj	2016-06-27 12:31:51.689755677 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr71626-1.c	2016-06-27 12:31:05.078344165 +0200
> @@ -0,0 +1,19 @@
> +/* PR middle-end/71626 */
> +
> +typedef __INTPTR_TYPE__ V __attribute__((__vector_size__(sizeof (__INTPTR_TYPE__))));
> +
> +__attribute__((noinline, noclone)) V
> +foo ()
> +{
> +  V v = { (__INTPTR_TYPE__) foo };
> +  return v;
> +}
> +
> +int
> +main ()
> +{
> +  V v = foo ();
> +  if (v[0] != (__INTPTR_TYPE__) foo)
> +    __builtin_abort ();
> +  return 0;
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr71626-2.c.jj	2016-06-27 12:31:54.837715933 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr71626-2.c	2016-06-27 12:31:47.977802542 +0200
> @@ -0,0 +1,4 @@
> +/* PR middle-end/71626 */
> +/* { dg-additional-options "-fpic" { target fpic } } */
> +
> +#include "pr71626-1.c"
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)