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

[<trevor_smigiel@playstation.sony.com>]

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