PR tree-opt/33614: ICE when optimising a "constant" vector constructor

PR tree-opt/33614: ICE when optimising a "constant" vector constructor

[Richard Sandiford]

This is first of a series of patches that started with the failure
of gcc.c-torture/compile/20050113-1.c on mipsisa32-elf targets.
The test contains a vector of the form:

    (V2SF) {1.0f/0.0f - 1.0f/0.0f, 1.0f/0.0f - 1.0f/0.0f};

(in a function context) and we seem to handle this in a strange way.
If we think 1.0f/0.0f - 1.0f/0.0f might trap, we don't fold it to a
constant.  However, we still mark the RDIV_EXPR as both TREE_CONSTANT
and TREE_INVARIANT.  This in turn causes us to mark the CONSTRUCTOR
that contains the RDIV_EXPRs as TREE_CONSTANT and TREE_INVARIANT too.

This seems odd on the face of it.  The only reason we haven't folded the
RDIV_EXPR to a constant is that we know it might trap.  Do we really
want to mark trapping operations as constant?  I assume we do, because
the handling in tree.c:build2_stat makes it look very deliberate.

The gimple infrastructure also embraces this "trapping values can be
constant" idea.  Vector constructors are gimplified like this:

	/* Go ahead and simplify constant constructors to VECTOR_CST.  */
	if (TREE_CONSTANT (ctor))
	  {
	    bool constant_p = true;
	    tree value;

	    /* Even when ctor is constant, it might contain non-*_CST
	      elements (e.g. { 1.0/0.0 - 1.0/0.0, 0.0 }) and those don't
	      belong into VECTOR_CST nodes.  */
	    FOR_EACH_CONSTRUCTOR_VALUE (elts, ix, value)
	      if (!CONSTANT_CLASS_P (value))
		{
		  constant_p = false;
		  break;
		}

	    if (constant_p)
	      {
		TREE_OPERAND (*expr_p, 1) = build_vector_from_ctor (type, elts);
		break;
	      }

	    /* Don't reduce a TREE_CONSTANT vector ctor even if we can't
	       make a VECTOR_CST.  It won't do anything for us, and it'll
	       prevent us from representing it as a single constant.  */
	    break;
	  }

i.e. we preserve a TREE_CONSTANT, TREE_INVARIANT vector CONSTRUCTOR
specifically in the case where one of the elements might trap.
(In the case of -fnon-call-exceptions, they might throw as well.)
We also specifically consider such constructors to satisfy
is_gimple_min_invariant:

    /* Vector constant constructors are gimple invariant.  */
    case CONSTRUCTOR:
      if (TREE_TYPE (t) && TREE_CODE (TREE_TYPE (t)) == VECTOR_TYPE)
	return TREE_CONSTANT (t);
      else
	return false;

All other invariants can be safely evaluated anywhere in the function,
and it seems odd to say that a trapping and potentially-throwing value
is also invariant.  My first attempt at showing this was a potential
problem was the testcase below:

-----------------------------------------------------------------------
typedef float V2SF __attribute__ ((vector_size (8)));

V2SF
foo (int x, V2SF a)
{
  while (x--)
    a += (V2SF) {1.0f/0.0f - 1.0f/0.0f, 1.0f/0.0f - 1.0f/0.0f};
  return a;
}
-----------------------------------------------------------------------

which in retrospect probably wouldn't have helped, since loop-header
copying would make any hoisting safe.  However, the testcase actually
produced an ICE:

-----------------------------------------------------------------------
/tmp/foo.c: In function 'foo':
/tmp/foo.c:9: error: invalid reference prefix
{1.0e+0 / 0.0 + 1.0e+0 / -0.0, 1.0e+0 / 0.0 + 1.0e+0 / -0.0}

/tmp/foo.c:9: error: invalid reference prefix
{1.0e+0 / 0.0 + 1.0e+0 / -0.0, 1.0e+0 / 0.0 + 1.0e+0 / -0.0}

/tmp/foo.c:9: internal compiler error: verify_stmts failed
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
-----------------------------------------------------------------------

Also, tree_could_trap_p and tree_could_throw_p never return true for
these invariant CONSTRUCTORs, so with that and the ICE, we have at
least two known problems.

I suppose the question is whether we should fix things using the current
gimple scheme or not.  To be honest, I don't really see what the current
scheme is trying to achieve.  The only reason we haven't folded this
constructor to a constant is that we know we'll have to evaluate some
part of it.  Why not expose that separate evaluation at the gimple
level, just like we do for all other non-constant vector constructors?
Exposing the separate evaluation also plugs the tree_could_trap_p and
tree_could_throw_p hole; there'd be no need to add separate CONSTRUCTOR
handling to them.

(AIUI, even TREE_CONSTANT CONSTRUCTORs are unique, and represent
distinct objects, so I don't think there's any need to copy them
before gimplifying the elements.)

Bootstrapped & regression-tested on x86_64-linux-gnu.  Also
regression-tested on mipsisa32-elf.  OK to install?

Richard


gcc/
	PR tree-optimization/33614
	* gimplify.c (gimplify_init_constructor): Gimplify vector
	constructors if we cannot turn them into a VECTOR_CST.
	* tree-cfg.c (verify_expr): Remove special CONSTRUCTOR handling.
	* tree-gimple.c (is_gimple_min_invariant): Likewise.

gcc/testsuite/
	PR tree-optimization/33614
	* gcc.c-torture/compile/pr33614.c: New test.

Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c	2007-10-01 23:23:20.000000000 +0100
+++ gcc/gimplify.c	2007-10-01 23:24:13.000000000 +0100
@@ -3291,10 +3291,8 @@ gimplify_init_constructor (tree *expr_p,
 		break;
 	      }
 
-	    /* Don't reduce a TREE_CONSTANT vector ctor even if we can't
-	       make a VECTOR_CST.  It won't do anything for us, and it'll
-	       prevent us from representing it as a single constant.  */
-	    break;
+	    TREE_CONSTANT (ctor) = 0;
+	    TREE_INVARIANT (ctor) = 0;
 	  }
 
 	/* Vector types use CONSTRUCTOR all the way through gimple
Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c	2007-10-01 23:23:20.000000000 +0100
+++ gcc/tree-cfg.c	2007-10-01 23:24:13.000000000 +0100
@@ -3348,11 +3348,6 @@ #define CHECK_OP(N, MSG) \
       CHECK_OP (1, "invalid operand to binary operator");
       break;
 
-    case CONSTRUCTOR:
-      if (TREE_CONSTANT (t) && TREE_CODE (TREE_TYPE (t)) == VECTOR_TYPE)
-	*walk_subtrees = 0;
-      break;
-
     default:
       break;
     }
Index: gcc/tree-gimple.c
===================================================================
--- gcc/tree-gimple.c	2007-10-01 23:23:20.000000000 +0100
+++ gcc/tree-gimple.c	2007-10-01 23:24:13.000000000 +0100
@@ -185,13 +185,6 @@ is_gimple_min_invariant (const_tree t)
     case VECTOR_CST:
       return true;
 
-    /* Vector constant constructors are gimple invariant.  */
-    case CONSTRUCTOR:
-      if (TREE_TYPE (t) && TREE_CODE (TREE_TYPE (t)) == VECTOR_TYPE)
-	return TREE_CONSTANT (t);
-      else
-	return false;
-
     default:
       return false;
     }
Index: gcc/testsuite/gcc.c-torture/compile/pr33614.c
===================================================================
--- /dev/null	2007-09-27 09:37:00.556097250 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr33614.c	2007-10-01 23:24:13.000000000 +0100
@@ -0,0 +1,9 @@
+typedef float V2SF __attribute__ ((vector_size (8)));
+
+V2SF
+foo (int x, V2SF a)
+{
+  while (x--)
+    a += (V2SF) {1.0f/0.0f - 1.0f/0.0f, 1.0f/0.0f - 1.0f/0.0f};
+  return a;
+}

Re: PR tree-opt/33614: ICE when optimising a "constant" vector constructor

[Andrew Pinski]

On 10/1/07, Richard Sandiford <rsandifo@nildram.co.uk> wrote:

> Index: gcc/gimplify.c
> ===================================================================
> --- gcc/gimplify.c      2007-10-01 23:23:20.000000000 +0100
> +++ gcc/gimplify.c      2007-10-01 23:24:13.000000000 +0100
> @@ -3291,10 +3291,8 @@ gimplify_init_constructor (tree *expr_p,
>                 break;
>               }
>
> -           /* Don't reduce a TREE_CONSTANT vector ctor even if we can't
> -              make a VECTOR_CST.  It won't do anything for us, and it'll
> -              prevent us from representing it as a single constant.  */
> -           break;
> +           TREE_CONSTANT (ctor) = 0;
> +           TREE_INVARIANT (ctor) = 0;
>           }
>
>         /* Vector types use CONSTRUCTOR all the way through gimple

Doing this will loose some optimizations with vectors.  The main
reason why is because &a is not a CONSTANT_CLASS but still can be
constant..

Also can you look at the following testcase (with -O2 -msse):
int t[4];

__attribute__((vector_size(16))) int f(void)
{
__attribute__((vector_size(16))) int t1 = {(int)&t[0], (int)&t[1], (int)&t[2],
(int)&t[3]};
  return t1;
}

And make sure we don't regress.
Right now we get:
f:
        pushl   %ebp
        movdqa  .LC0, %xmm0
        popl    %ebp
        ret
.LC0:
        .long   t
        .long   t+4
        .long   t+8
        .long   t+12
Which is better than what we got in 4.0.2.

Re: PR tree-opt/33614: ICE when optimising a "constant" vector constructor

[Andrew Pinski]

On 10/1/07, Andrew Pinski <pinskia@gmail.com> wrote:
> Also can you look at the following testcase (with -O2 -msse):
> int t[4];
>
> __attribute__((vector_size(16))) int f(void)
> {
> __attribute__((vector_size(16))) int t1 = {(int)&t[0], (int)&t[1], (int)&t[2],
> (int)&t[3]};
>   return t1;
> }

Or the following testcase for PPC:
int t[4];
__attribute__((vector_size(16))) int g;

void f(void)
{
__attribute__((vector_size(16))) int t1 = {(int)&t[0], (int)&t[1], (int)&t[2],
(int)&t[3]};
g = t1;
}

---------- CUT ---------------------
Right now we get:
        lis 9,.LANCHOR0@ha
        lis 11,g@ha
        la 9,.LANCHOR0@l(9)
        la 11,g@l(11)
        lvx 0,0,9
        stvx 0,0,11
        blr

But with your patch, we will not force the constant vector into
constant memory so we get at least one load hit store hazzard (on the
Cell) as we will be storing out to the stack the vector and then
loading it back up.

Thanks,
Andrew Pinski

Re: PR tree-opt/33614: ICE when optimising a "constant" vector constructor

[Richard Sandiford]

"Andrew Pinski" <pinskia@gmail.com> writes:
> On 10/1/07, Richard Sandiford <rsandifo@nildram.co.uk> wrote:
>
>> Index: gcc/gimplify.c
>> ===================================================================
>> --- gcc/gimplify.c      2007-10-01 23:23:20.000000000 +0100
>> +++ gcc/gimplify.c      2007-10-01 23:24:13.000000000 +0100
>> @@ -3291,10 +3291,8 @@ gimplify_init_constructor (tree *expr_p,
>>                 break;
>>               }
>>
>> -           /* Don't reduce a TREE_CONSTANT vector ctor even if we can't
>> -              make a VECTOR_CST.  It won't do anything for us, and it'll
>> -              prevent us from representing it as a single constant.  */
>> -           break;
>> +           TREE_CONSTANT (ctor) = 0;
>> +           TREE_INVARIANT (ctor) = 0;
>>           }
>>
>>         /* Vector types use CONSTRUCTOR all the way through gimple
>
> Doing this will loose some optimizations with vectors.  The main
> reason why is because &a is not a CONSTANT_CLASS but still can be
> constant..

OK.  How about keeping the break when none of the elements can throw?
And updating the comment so that it mentions this case too?

> Also can you look at the following testcase (with -O2 -msse):
> int t[4];
>
> __attribute__((vector_size(16))) int f(void)
> {
> __attribute__((vector_size(16))) int t1 = {(int)&t[0], (int)&t[1], (int)&t[2],
> (int)&t[3]};
>   return t1;
> }
>
> And make sure we don't regress.
> Right now we get:
> f:
>         pushl   %ebp
>         movdqa  .LC0, %xmm0
>         popl    %ebp
>         ret
> .LC0:
>         .long   t
>         .long   t+4
>         .long   t+8
>         .long   t+12
> Which is better than what we got in 4.0.2.

FWIW, that particular case still works.

Richard

Re: PR tree-opt/33614: ICE when optimising a "constant" vector constructor

[Richard Sandiford]

"Andrew Pinski" <pinskia@gmail.com> writes:
> On 10/1/07, Andrew Pinski <pinskia@gmail.com> wrote:
>> Also can you look at the following testcase (with -O2 -msse):
>> int t[4];
>>
>> __attribute__((vector_size(16))) int f(void)
>> {
>> __attribute__((vector_size(16))) int t1 = {(int)&t[0], (int)&t[1], (int)&t[2],
>> (int)&t[3]};
>>   return t1;
>> }
>
> Or the following testcase for PPC:
> int t[4];
> __attribute__((vector_size(16))) int g;
>
> void f(void)
> {
> __attribute__((vector_size(16))) int t1 = {(int)&t[0], (int)&t[1], (int)&t[2],
> (int)&t[3]};
> g = t1;
> }
>
> ---------- CUT ---------------------
> Right now we get:
>         lis 9,.LANCHOR0@ha
>         lis 11,g@ha
>         la 9,.LANCHOR0@l(9)
>         la 11,g@l(11)
>         lvx 0,0,9
>         stvx 0,0,11
>         blr
>
> But with your patch, we will not force the constant vector into
> constant memory so we get at least one load hit store hazzard (on the
> Cell) as we will be storing out to the stack the vector and then
> loading it back up.

OK, you've convinced me that this is deeper than I first realised.
I was misled by the gimplify.c comment, which implied that it was
only dealing with trapping cases.

It'd probably best if I unassign myself from this one.  It's really
33616 that I'm trying to fix.

Richard