[PATCH] Fix ICE in simplify_immed_subreg on SPU

[PATCH] Fix ICE in simplify_immed_subreg on SPU

[Ulrich Weigand]

Hello,

the test cases for PR 34856 ICE on SPU because simplify_subreg is 
called on a CONST_VECTOR RTX of the form:

(const_vector:V4SI [
        (const:SI (plus:SI (symbol_ref:SI ("g") [flags 0x40] <var_decl 0xf70b05a0 g>)
                (const_int 32 [0x20])))
        (const_int 0 [0x0])
        (const_int 0 [0x0])
        (const_int 0 [0x0])
    ])

and everything but CONST_INT, CONST_DOUBLE or CONST_FIXED within
a CONST_VECTOR leads to an ICE in simplify_immed_subreg.

It seems to me that having an address constant like the above appear
within a CONST_VECTOR should be OK -- it just cannot be simplified
by simplify_immed_subreg so that function should return NULL_RTX.

The patch below implements this.  Tested on spu-elf with no regressions,
fixing the following test suite FAILs:

FAIL: gcc.c-torture/compile/pr34856.c  -O2  (internal compiler error)
FAIL: gcc.c-torture/compile/pr34856.c  -O2  (test for excess errors)
FAIL: gcc.c-torture/compile/pr34856.c  -O3 -fomit-frame-pointer  (internal compiler error)
FAIL: gcc.c-torture/compile/pr34856.c  -O3 -fomit-frame-pointer  (test for excess errors)
FAIL: gcc.c-torture/compile/pr34856.c  -O3 -fomit-frame-pointer -funroll-loops  (internal compiler error)
FAIL: gcc.c-torture/compile/pr34856.c  -O3 -fomit-frame-pointer -funroll-loops  (test for excess errors)
FAIL: gcc.c-torture/compile/pr34856.c  -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions  (internal compiler error)
FAIL: gcc.c-torture/compile/pr34856.c  -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions  (test for excess errors)
FAIL: gcc.c-torture/compile/pr34856.c  -O3 -g  (internal compiler error)
FAIL: gcc.c-torture/compile/pr34856.c  -O3 -g  (test for excess errors)
FAIL: gcc.c-torture/compile/pr34856.c  -Os  (internal compiler error)
FAIL: gcc.c-torture/compile/pr34856.c  -Os  (test for excess errors)
FAIL: gcc.dg/pr34856.c (internal compiler error)
FAIL: gcc.dg/pr34856.c (test for excess errors)


OK for mainline and 4.3 branch?

Bye,
Ulrich


ChangeLog:

	* simplify-rtx.c (simplify_immed_subreg): Do not ICE when seeing
	a CONST_VECTOR with an address constant as element.


Index: gcc/simplify-rtx.c
===================================================================
*** gcc/simplify-rtx.c	(revision 136680)
--- gcc/simplify-rtx.c	(working copy)
*************** simplify_immed_subreg (enum machine_mode
*** 4792,4798 ****
  		*vp++ = 0;
  	    }
            break;
! 	  
  	default:
  	  gcc_unreachable ();
  	}
--- 4792,4804 ----
  		*vp++ = 0;
  	    }
            break;
! 
! 	case CONST:
! 	case SYMBOL_REF:
! 	case LABEL_REF:
! 	  /* We cannot decompose an address constant.  */
! 	  return NULL_RTX;
! 
  	default:
  	  gcc_unreachable ();
  	}
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Eric Botcazou]

> the test cases for PR 34856 ICE on SPU because simplify_subreg is
> called on a CONST_VECTOR RTX of the form:
>
> (const_vector:V4SI [
>         (const:SI (plus:SI (symbol_ref:SI ("g") [flags 0x40] <var_decl
> 0xf70b05a0 g>) (const_int 32 [0x20])))
>         (const_int 0 [0x0])
>         (const_int 0 [0x0])
>         (const_int 0 [0x0])
>     ])
>
> and everything but CONST_INT, CONST_DOUBLE or CONST_FIXED within
> a CONST_VECTOR leads to an ICE in simplify_immed_subreg.
>
> It seems to me that having an address constant like the above appear
> within a CONST_VECTOR should be OK -- it just cannot be simplified
> by simplify_immed_subreg so that function should return NULL_RTX.

No, we already ran into this on x86, CONST_VECTOR should only contain the 
aforementioned 3 kinds of constants.

-- 
Eric Botcazou

Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Ulrich Weigand]

Eric Botcazou wrote:

> > It seems to me that having an address constant like the above appear
> > within a CONST_VECTOR should be OK -- it just cannot be simplified
> > by simplify_immed_subreg so that function should return NULL_RTX.
> 
> No, we already ran into this on x86, CONST_VECTOR should only contain the 
> aforementioned 3 kinds of constants.

Why is this?  It seems constructing the vector in the constant pool
and loading it from there is the most efficient way to load this
particular vector on the SPU -- but I don't see how to express this
in a different manner without using a CONST_VECTOR ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Eric Botcazou]

> Why is this?  It seems constructing the vector in the constant pool
> and loading it from there is the most efficient way to load this
> particular vector on the SPU -- but I don't see how to express this
> in a different manner without using a CONST_VECTOR ...

See the manual about CONST_VECTOR.  Isn't the vector initializer already put 
in the constant pool as a CONSTRUCTOR at the tree level?

-- 
Eric Botcazou

Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Ian Lance Taylor]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:

> Eric Botcazou wrote:
>
>> > It seems to me that having an address constant like the above appear
>> > within a CONST_VECTOR should be OK -- it just cannot be simplified
>> > by simplify_immed_subreg so that function should return NULL_RTX.
>> 
>> No, we already ran into this on x86, CONST_VECTOR should only contain the 
>> aforementioned 3 kinds of constants.
>
> Why is this?  It seems constructing the vector in the constant pool
> and loading it from there is the most efficient way to load this
> particular vector on the SPU -- but I don't see how to express this
> in a different manner without using a CONST_VECTOR ...

I think I'm with Ulrich here.  A CONST_VECTOR containing a CONST seems
reasonable to me.  Eric, can you clarify what you see as the problem?

Ian

Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Eric Botcazou]

> I think I'm with Ulrich here.  A CONST_VECTOR containing a CONST seems
> reasonable to me.  Eric, can you clarify what you see as the problem?

I don't see the point in changing the semantics of CONST_VECTOR because of
a IMO non-representative testcase.

-- 
Eric Botcazou

Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Andrew Pinski]

Sent from my iPhone

On Jun 15, 2008, at 23:27, Eric Botcazou <ebotcazou@adacore.com> wrote:

>> I think I'm with Ulrich here.  A CONST_VECTOR containing a CONST  
>> seems
>> reasonable to me.  Eric, can you clarify what you see as the problem?
>
> I don't see the point in changing the semantics of CONST_VECTOR  
> because of
> a IMO non-representative testcase.
>

This testcase came from real SPU code in the wild. The testcase was  
derived from the PS3 OS sources.

-- Pinski

[PATCH, v2] Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Ulrich Weigand]

Eric Botcazou wrote:

> > I think I'm with Ulrich here.  A CONST_VECTOR containing a CONST seems
> > reasonable to me.  Eric, can you clarify what you see as the problem?
> 
> I don't see the point in changing the semantics of CONST_VECTOR because of
> a IMO non-representative testcase.

I've looked at this again in more detail, and it seems that there is
indeed the pervasive assumption that CONST_VECTOR elements can only
be CONST_INT, CONST_DOUBLE, and CONST_FIXED.  For example, dwarf2out.c
cannot handle anything else when constructing an DW_AT_const_value
attribute refering to a CONST_VECTOR.

While I still think it would be better to allow any legitimate constant
as member of a CONST_VECTOR, this would require non-trivial changes to
dwarf2out.c and a couple of other places.  Therefore, for now I think
it is best to change the SPU back-end to avoid generating CONST_VECTOR
expressions that are invalid according to the current middle-end rules.
At some later time those middle-end rules could be relaxed ...

The following (SPU back-end only) patch implements this.  While this
might result in somewhat less efficient code being generated for
certain case, those cases should be rather rare.

Trevor/Andrew, what are your thoughts about this?  Would this be OK
for mainline and 4.3?

Tested on spu-elf with no regressions, fixes the pr34856 FAILs.

Bye,
Ulrich


ChangeLog:

	PR target/34856
	* config/spu/spu.c (spu_builtin_splats): Do not generate
	invalid CONST_VECTOR expressions.
	(spu_expand_vector_init): Likewise.


Index: gcc/config/spu/spu.c
===================================================================
*** gcc/config/spu/spu.c	(revision 136912)
--- gcc/config/spu/spu.c	(working copy)
*************** spu_builtin_splats (rtx ops[])
*** 4576,4590 ****
        constant_to_array (GET_MODE_INNER (mode), ops[1], arr);
        emit_move_insn (ops[0], array_to_constant (mode, arr));
      }
-   else if (!flag_pic && GET_MODE (ops[0]) == V4SImode && CONSTANT_P (ops[1]))
-     {
-       rtvec v = rtvec_alloc (4);
-       RTVEC_ELT (v, 0) = ops[1];
-       RTVEC_ELT (v, 1) = ops[1];
-       RTVEC_ELT (v, 2) = ops[1];
-       RTVEC_ELT (v, 3) = ops[1];
-       emit_move_insn (ops[0], gen_rtx_CONST_VECTOR (mode, v));
-     }
    else
      {
        rtx reg = gen_reg_rtx (TImode);
--- 4576,4581 ----
*************** spu_expand_vector_init (rtx target, rtx 
*** 4903,4909 ****
    for (i = 0; i < n_elts; ++i)
      {
        x = XVECEXP (vals, 0, i);
!       if (!CONSTANT_P (x))
  	++n_var;
        else
  	{
--- 4894,4902 ----
    for (i = 0; i < n_elts; ++i)
      {
        x = XVECEXP (vals, 0, i);
!       if (!(CONST_INT_P (x)
! 	    || GET_CODE (x) == CONST_DOUBLE
! 	    || GET_CODE (x) == CONST_FIXED))
  	++n_var;
        else
  	{
*************** spu_expand_vector_init (rtx target, rtx 
*** 4940,4947 ****
  	  /* fill empty slots with the first constant, this increases
  	     our chance of using splats in the recursive call below. */
  	  for (i = 0; i < n_elts; ++i)
! 	    if (!CONSTANT_P (XVECEXP (constant_parts_rtx, 0, i)))
! 	      XVECEXP (constant_parts_rtx, 0, i) = first_constant;
  
  	  spu_expand_vector_init (target, constant_parts_rtx);
  	}
--- 4933,4945 ----
  	  /* fill empty slots with the first constant, this increases
  	     our chance of using splats in the recursive call below. */
  	  for (i = 0; i < n_elts; ++i)
! 	    {
! 	      x = XVECEXP (constant_parts_rtx, 0, i);
! 	      if (!(CONST_INT_P (x)
! 		    || GET_CODE (x) == CONST_DOUBLE
! 		    || GET_CODE (x) == CONST_FIXED))
! 		XVECEXP (constant_parts_rtx, 0, i) = first_constant;
! 	    }
  
  	  spu_expand_vector_init (target, constant_parts_rtx);
  	}
*************** spu_expand_vector_init (rtx target, rtx 
*** 4957,4963 ****
        for (i = 0; i < n_elts; ++i)
  	{
  	  x = XVECEXP (vals, 0, i);
! 	  if (!CONSTANT_P (x))
  	    {
  	      if (!register_operand (x, GET_MODE (x)))
  		x = force_reg (GET_MODE (x), x);
--- 4955,4963 ----
        for (i = 0; i < n_elts; ++i)
  	{
  	  x = XVECEXP (vals, 0, i);
! 	  if (!(CONST_INT_P (x)
! 		|| GET_CODE (x) == CONST_DOUBLE
! 		|| GET_CODE (x) == CONST_FIXED))
  	    {
  	      if (!register_operand (x, GET_MODE (x)))
  		x = force_reg (GET_MODE (x), x);

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, v2] Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[trevor_smigiel]

Ulrich,

I have no additional information to refute the opinion that our usage is
"non-representative".  We have code that uses these semantics, so it is
useful to us.

The patch is ok.  Go ahead and check it in.

Trevor


* Ulrich Weigand <uweigand@de.ibm.com> [2008-06-26 15:15]:
> Eric Botcazou wrote:
> 
> > > I think I'm with Ulrich here.  A CONST_VECTOR containing a CONST seems
> > > reasonable to me.  Eric, can you clarify what you see as the problem?
> > 
> > I don't see the point in changing the semantics of CONST_VECTOR because of
> > a IMO non-representative testcase.
> 
> I've looked at this again in more detail, and it seems that there is
> indeed the pervasive assumption that CONST_VECTOR elements can only
> be CONST_INT, CONST_DOUBLE, and CONST_FIXED.  For example, dwarf2out.c
> cannot handle anything else when constructing an DW_AT_const_value
> attribute refering to a CONST_VECTOR.
> 
> While I still think it would be better to allow any legitimate constant
> as member of a CONST_VECTOR, this would require non-trivial changes to
> dwarf2out.c and a couple of other places.  Therefore, for now I think
> it is best to change the SPU back-end to avoid generating CONST_VECTOR
> expressions that are invalid according to the current middle-end rules.
> At some later time those middle-end rules could be relaxed ...
> 
> The following (SPU back-end only) patch implements this.  While this
> might result in somewhat less efficient code being generated for
> certain case, those cases should be rather rare.
> 
> Trevor/Andrew, what are your thoughts about this?  Would this be OK
> for mainline and 4.3?
> 
> Tested on spu-elf with no regressions, fixes the pr34856 FAILs.
> 
> Bye,
> Ulrich
> 
> 
> ChangeLog:
> 
> 	PR target/34856
> 	* config/spu/spu.c (spu_builtin_splats): Do not generate
> 	invalid CONST_VECTOR expressions.
> 	(spu_expand_vector_init): Likewise.
> 
> 
> Index: gcc/config/spu/spu.c
> ===================================================================
> *** gcc/config/spu/spu.c	(revision 136912)
> --- gcc/config/spu/spu.c	(working copy)
> *************** spu_builtin_splats (rtx ops[])
> *** 4576,4590 ****
>         constant_to_array (GET_MODE_INNER (mode), ops[1], arr);
>         emit_move_insn (ops[0], array_to_constant (mode, arr));
>       }
> -   else if (!flag_pic && GET_MODE (ops[0]) == V4SImode && CONSTANT_P (ops[1]))
> -     {
> -       rtvec v = rtvec_alloc (4);
> -       RTVEC_ELT (v, 0) = ops[1];
> -       RTVEC_ELT (v, 1) = ops[1];
> -       RTVEC_ELT (v, 2) = ops[1];
> -       RTVEC_ELT (v, 3) = ops[1];
> -       emit_move_insn (ops[0], gen_rtx_CONST_VECTOR (mode, v));
> -     }
>     else
>       {
>         rtx reg = gen_reg_rtx (TImode);
> --- 4576,4581 ----
> *************** spu_expand_vector_init (rtx target, rtx 
> *** 4903,4909 ****
>     for (i = 0; i < n_elts; ++i)
>       {
>         x = XVECEXP (vals, 0, i);
> !       if (!CONSTANT_P (x))
>   	++n_var;
>         else
>   	{
> --- 4894,4902 ----
>     for (i = 0; i < n_elts; ++i)
>       {
>         x = XVECEXP (vals, 0, i);
> !       if (!(CONST_INT_P (x)
> ! 	    || GET_CODE (x) == CONST_DOUBLE
> ! 	    || GET_CODE (x) == CONST_FIXED))
>   	++n_var;
>         else
>   	{
> *************** spu_expand_vector_init (rtx target, rtx 
> *** 4940,4947 ****
>   	  /* fill empty slots with the first constant, this increases
>   	     our chance of using splats in the recursive call below. */
>   	  for (i = 0; i < n_elts; ++i)
> ! 	    if (!CONSTANT_P (XVECEXP (constant_parts_rtx, 0, i)))
> ! 	      XVECEXP (constant_parts_rtx, 0, i) = first_constant;
>   
>   	  spu_expand_vector_init (target, constant_parts_rtx);
>   	}
> --- 4933,4945 ----
>   	  /* fill empty slots with the first constant, this increases
>   	     our chance of using splats in the recursive call below. */
>   	  for (i = 0; i < n_elts; ++i)
> ! 	    {
> ! 	      x = XVECEXP (constant_parts_rtx, 0, i);
> ! 	      if (!(CONST_INT_P (x)
> ! 		    || GET_CODE (x) == CONST_DOUBLE
> ! 		    || GET_CODE (x) == CONST_FIXED))
> ! 		XVECEXP (constant_parts_rtx, 0, i) = first_constant;
> ! 	    }
>   
>   	  spu_expand_vector_init (target, constant_parts_rtx);
>   	}
> *************** spu_expand_vector_init (rtx target, rtx 
> *** 4957,4963 ****
>         for (i = 0; i < n_elts; ++i)
>   	{
>   	  x = XVECEXP (vals, 0, i);
> ! 	  if (!CONSTANT_P (x))
>   	    {
>   	      if (!register_operand (x, GET_MODE (x)))
>   		x = force_reg (GET_MODE (x), x);
> --- 4955,4963 ----
>         for (i = 0; i < n_elts; ++i)
>   	{
>   	  x = XVECEXP (vals, 0, i);
> ! 	  if (!(CONST_INT_P (x)
> ! 		|| GET_CODE (x) == CONST_DOUBLE
> ! 		|| GET_CODE (x) == CONST_FIXED))
>   	    {
>   	      if (!register_operand (x, GET_MODE (x)))
>   		x = force_reg (GET_MODE (x), x);
> 
> -- 
>   Dr. Ulrich Weigand
>   GNU Toolchain for Linux on System z and Cell BE
>   Ulrich.Weigand@de.ibm.com

Re: [PATCH, v2] Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Eric Botcazou]

> While I still think it would be better to allow any legitimate constant
> as member of a CONST_VECTOR, this would require non-trivial changes to
> dwarf2out.c and a couple of other places.  Therefore, for now I think
> it is best to change the SPU back-end to avoid generating CONST_VECTOR
> expressions that are invalid according to the current middle-end rules.

Thanks for investigating.  While you're at it, could you add CONST_FIXED to 
the list of allowed elements for a CONST_VECTOR in rtl.texi?

-- 
Eric Botcazou

Re: [PATCH, v2] Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Ulrich Weigand]

Eric Botcazou wrote:

> Thanks for investigating.  While you're at it, could you add CONST_FIXED to 
> the list of allowed elements for a CONST_VECTOR in rtl.texi?

Sure.  I've now committed the following patch.

Bye,
Ulrich


2008-06-28  Ulrich Weigand  <Ulrich.Weigand@de.ibm.com>

	* doc/rtl.texi (const_vector): Document const_fixed as legitimate
	element type of const_vector.

Index: gcc/doc/rtl.texi
===================================================================
--- gcc/doc/rtl.texi	(revision 137229)
+++ gcc/doc/rtl.texi	(working copy)
@@ -1527,7 +1527,7 @@
 @item (const_vector:@var{m} [@var{x0} @var{x1} @dots{}])
 Represents a vector constant.  The square brackets stand for the vector
 containing the constant elements.  @var{x0}, @var{x1} and so on are
-the @code{const_int} or @code{const_double} elements.
+the @code{const_int}, @code{const_double} or @code{const_fixed} elements.
 
 The number of units in a @code{const_vector} is obtained with the macro
 @code{CONST_VECTOR_NUNITS} as in @code{CONST_VECTOR_NUNITS (@var{v})}.

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, v2] Re: [PATCH] Fix ICE in simplify_immed_subreg on SPU

[Eric Botcazou]

> Sure.  I've now committed the following patch.

Thanks!

-- 
Eric Botcazou