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

[Richard Sandiford <rsandifo at nildram dot co dot uk>]

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;
+}