diff mbox series

c++: Don't shortcut TREE_CONSTANT vector type CONSTRUCTORs in cxx_eval_constant_expression [PR107295]

Message ID Y0+rxzMBPmFcWzqe@tucnak
State Unresolved
Headers
Series c++: Don't shortcut TREE_CONSTANT vector type CONSTRUCTORs in cxx_eval_constant_expression [PR107295] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Oct. 19, 2022, 7:48 a.m. UTC 
  Hi!

The excess precision support broke building skia (dependency of firefox)
on ia32 (it has something like the a constexpr variable), but as the other
cases show, it is actually a preexisting problem if one uses casts from
constants with wider floating point types.
The problem is that cxx_eval_constant_expression tries to short-cut
processing of TREE_CONSTANT CONSTRUCTORs if they satisfy
reduced_constant_expression_p - instead of calling cxx_eval_bare_aggregate
on them it just verifies flags and if they are TREE_CONSTANT even after
that, just fold.
Now, on the testcase we have a TREE_CONSTANT CONSTRUCTOR containing
TREE_CONSTANT NOP_EXPR of REAL_CST.  And, fold, which isn't recursive,
doesn't optimize that into VECTOR_CST, while later on we are only able
to optimize VECTOR_CST arithmetics, not arithmetics with vector
CONSTRUCTORs.
The following patch fixes that by only returning what fold returned
if for vector types it returned VECTOR_CST, otherwise let us
call cxx_eval_bare_aggregate.  That function will try to constant
evaluate all the elements and if anything changes, return a CONSTRUCTOR,
in the vector type cases with fold called on it at the end.
Now, just calling cxx_eval_bare_aggregate for vector types doesn't work
either (e.g. constexpr-builtin4.C breaks), because cxx_eval_bare_aggregate
if nothing changes (like all elts are already REAL_CSTs or INTEGER_CSTs)
will return the old CONSTRUCTOR and nothing folds it into a VECTOR_CST.
Also, the reason for the short-cutting is I think trying to avoid
allocating a new CONSTRUCTOR when nothing changes and we just create
GC garbage by it.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2022-10-19  Jakub Jelinek  <jakub@redhat.com>

	PR c++/107295
	* constexpr.cc (cxx_eval_constant_expression) <case CONSTRUCTOR>:
	Don't short-cut TREE_CONSTANT vector ctors if fold doesn't turn them
	into VECTOR_CST.

	* g++.dg/ext/vector42.C: New test.


	Jakub

Comments

Jason Merrill Oct. 20, 2022, 2:51 p.m. UTC | #1 
On 10/19/22 03:48, Jakub Jelinek wrote:
> Hi!
> 
> The excess precision support broke building skia (dependency of firefox)
> on ia32 (it has something like the a constexpr variable), but as the other
> cases show, it is actually a preexisting problem if one uses casts from
> constants with wider floating point types.
> The problem is that cxx_eval_constant_expression tries to short-cut
> processing of TREE_CONSTANT CONSTRUCTORs if they satisfy
> reduced_constant_expression_p - instead of calling cxx_eval_bare_aggregate
> on them it just verifies flags and if they are TREE_CONSTANT even after
> that, just fold.
> Now, on the testcase we have a TREE_CONSTANT CONSTRUCTOR containing
> TREE_CONSTANT NOP_EXPR of REAL_CST.  And, fold, which isn't recursive,
> doesn't optimize that into VECTOR_CST, while later on we are only able
> to optimize VECTOR_CST arithmetics, not arithmetics with vector
> CONSTRUCTORs.
> The following patch fixes that by only returning what fold returned
> if for vector types it returned VECTOR_CST, otherwise let us
> call cxx_eval_bare_aggregate.  That function will try to constant
> evaluate all the elements and if anything changes, return a CONSTRUCTOR,
> in the vector type cases with fold called on it at the end.
> Now, just calling cxx_eval_bare_aggregate for vector types doesn't work
> either (e.g. constexpr-builtin4.C breaks), because cxx_eval_bare_aggregate
> if nothing changes (like all elts are already REAL_CSTs or INTEGER_CSTs)
> will return the old CONSTRUCTOR and nothing folds it into a VECTOR_CST.

That seems like a bug; for VECTOR_TYPE we should fold even if !changed.

> Also, the reason for the short-cutting is I think trying to avoid
> allocating a new CONSTRUCTOR when nothing changes and we just create
> GC garbage by it.

We might limit the shortcut to non-vector types by hoisting the vector 
check in reduced_constant_expression_p out of the 
CONSTRUCTOR_NO_CLEARING condition:

>       if (CONSTRUCTOR_NO_CLEARING (t))
>         {
>           if (TREE_CODE (TREE_TYPE (t)) == VECTOR_TYPE)
>             /* An initialized vector would have a VECTOR_CST.  */
>             return false;

then we could remove the fold in the shortcut.

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2022-10-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/107295
> 	* constexpr.cc (cxx_eval_constant_expression) <case CONSTRUCTOR>:
> 	Don't short-cut TREE_CONSTANT vector ctors if fold doesn't turn them
> 	into VECTOR_CST.
> 
> 	* g++.dg/ext/vector42.C: New test.
> 
> --- gcc/cp/constexpr.cc.jj	2022-10-17 12:29:33.518016420 +0200
> +++ gcc/cp/constexpr.cc	2022-10-19 01:29:28.761935708 +0200
> @@ -7391,7 +7391,12 @@ cxx_eval_constant_expression (const cons
>   	     VECTOR_CST if applicable.  */
>   	  verify_constructor_flags (t);
>   	  if (TREE_CONSTANT (t))
> -	    return fold (t);
> +	    {
> +	      r = fold (t);
> +	      if (TREE_CODE (TREE_TYPE (t)) != VECTOR_TYPE
> +		  || TREE_CODE (r) == VECTOR_CST)
> +		return r;
> +	    }
>   	}
>         r = cxx_eval_bare_aggregate (ctx, t, lval,
>   				   non_constant_p, overflow_p);
> --- gcc/testsuite/g++.dg/ext/vector42.C.jj	2022-10-18 12:33:42.938510483 +0200
> +++ gcc/testsuite/g++.dg/ext/vector42.C	2022-10-18 12:32:27.448544476 +0200
> @@ -0,0 +1,12 @@
> +// PR c++/107295
> +// { dg-do compile { target c++11 } }
> +
> +template <typename T> struct A {
> +  typedef T __attribute__((vector_size (sizeof (int)))) V;
> +};
> +template <int, typename T> using B = typename A<T>::V;
> +template <typename T> using V = B<4, T>;
> +using F = V<float>;
> +constexpr F a = F () + 0.0f;
> +constexpr F b = F () + (float) 0.0;
> +constexpr F c = F () + (float) 0.0L;
> 
> 	Jakub
>
diff mbox series

Patch

--- gcc/cp/constexpr.cc.jj	2022-10-17 12:29:33.518016420 +0200
+++ gcc/cp/constexpr.cc	2022-10-19 01:29:28.761935708 +0200
@@ -7391,7 +7391,12 @@  cxx_eval_constant_expression (const cons
 	     VECTOR_CST if applicable.  */
 	  verify_constructor_flags (t);
 	  if (TREE_CONSTANT (t))
-	    return fold (t);
+	    {
+	      r = fold (t);
+	      if (TREE_CODE (TREE_TYPE (t)) != VECTOR_TYPE
+		  || TREE_CODE (r) == VECTOR_CST)
+		return r;
+	    }
 	}
       r = cxx_eval_bare_aggregate (ctx, t, lval,
 				   non_constant_p, overflow_p);
--- gcc/testsuite/g++.dg/ext/vector42.C.jj	2022-10-18 12:33:42.938510483 +0200
+++ gcc/testsuite/g++.dg/ext/vector42.C	2022-10-18 12:32:27.448544476 +0200
@@ -0,0 +1,12 @@ 
+// PR c++/107295
+// { dg-do compile { target c++11 } }
+
+template <typename T> struct A {
+  typedef T __attribute__((vector_size (sizeof (int)))) V;
+};
+template <int, typename T> using B = typename A<T>::V;
+template <typename T> using V = B<4, T>;
+using F = V<float>;
+constexpr F a = F () + 0.0f;
+constexpr F b = F () + (float) 0.0;
+constexpr F c = F () + (float) 0.0L;