[C++ PATCH] PR 70501, ICE in verify ctor sanity

[C++ PATCH] PR 70501, ICE in verify ctor sanity

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70501

This fixes 70501.  The cause is an omission in typeck when converting a scalar 
operand to a vector.  We use build_vector_from_val, which can return a 
CONSTRUCTOR.  We fail to wrap that CONSTRUCTOR in a TARGET_EXPR.

The ICE arises because at the point we meet that CONSTRUCTOR during the 
constexpr processing, the currently active object under construction is that for 
the result of the <= operator, which has type vector-of-bool, rather than 
vector-of-int. (thus this  problem arises in other vector  ops, but mostly 
undetected because the result type is the same  as the operand type)

ok?

nathan

[-- Attachment #2: 70501.patch --]
[-- Type: text/x-patch, Size: 1538 bytes --]

2016-04-04  Nathan Sidwell  <nathan@acm.org>

	PR c++/70501
	* typeck.c (cp_build_binary_op): Wrap vector constructors in
	target_exprs.

	* g++.dg/init/pr70501.C: New.

Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 234715)
+++ cp/typeck.c	(working copy)
@@ -4196,6 +4196,8 @@ cp_build_binary_op (location_t location,
               op0 = convert (TREE_TYPE (type1), op0);
 	      op0 = save_expr (op0);
               op0 = build_vector_from_val (type1, op0);
+	      if (TREE_CODE (op0) == CONSTRUCTOR)
+		op0 = get_target_expr (op0);
               type0 = TREE_TYPE (op0);
               code0 = TREE_CODE (type0);
               converted = 1;
@@ -4206,6 +4208,8 @@ cp_build_binary_op (location_t location,
               op1 = convert (TREE_TYPE (type0), op1);
 	      op1 = save_expr (op1);
               op1 = build_vector_from_val (type0, op1);
+	      if (TREE_CODE (op1) == CONSTRUCTOR)
+		op1 = get_target_expr (op1);
               type1 = TREE_TYPE (op1);
               code1 = TREE_CODE (type1);
               converted = 1;
Index: testsuite/g++.dg/init/pr70501.C
===================================================================
--- testsuite/g++.dg/init/pr70501.C	(nonexistent)
+++ testsuite/g++.dg/init/pr70501.C	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-options "" } Not pedantic */
+
+typedef int v4si __attribute__ ((vector_size (16)));
+
+struct S { v4si v; };
+
+void
+fn2 (int i, int j)
+{
+  struct S s = { .v = i <= j + (v4si){(1, 2)} };
+}

Re: [C++ PATCH] PR 70501, ICE in verify ctor sanity

[Jason Merrill]

On 04/04/2016 01:26 PM, Nathan Sidwell wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70501
>
> This fixes 70501.  The cause is an omission in typeck when converting a
> scalar operand to a vector.  We use build_vector_from_val, which can
> return a CONSTRUCTOR.  We fail to wrap that CONSTRUCTOR in a TARGET_EXPR.
>
> The ICE arises because at the point we meet that CONSTRUCTOR during the
> constexpr processing, the currently active object under construction is
> that for the result of the <= operator, which has type vector-of-bool,
> rather than vector-of-int. (thus this  problem arises in other vector
> ops, but mostly undetected because the result type is the same  as the
> operand type)

It's not clear to me that we really need a TARGET_EXPR for vector 
values.  Since one element of a vector can't refer to another, we don't 
need the ctx->ctor handling.  Perhaps we should handle vectors like we 
do PMF types in cxx_eval_bare_aggregate?

Jason

Re: [C++ PATCH] PR 70501, ICE in verify ctor sanity

[Nathan Sidwell]

On 04/05/16 12:40, Jason Merrill wrote:

> It's not clear to me that we really need a TARGET_EXPR for vector values.  Since
> one element of a vector can't refer to another, we don't need the ctx->ctor
> handling.  Perhaps we should handle vectors like we do PMF types in
> cxx_eval_bare_aggregate?

That may be abstractly better, but we do currently wrap constructors in 
target_exprs for vector compound_literals (which is what I was following).  See 
the get_target_expr_sfinae  calls in finish_compound_literal for instance.  That 
happens for  the '(v4si){(0, 0)}' subexpression of the testcase.

nathan

Re: [C++ PATCH] PR 70501, ICE in verify ctor sanity

[Jason Merrill]

On 04/05/2016 05:21 PM, Nathan Sidwell wrote:
> On 04/05/16 12:40, Jason Merrill wrote:
>
>> It's not clear to me that we really need a TARGET_EXPR for vector values.  Since
>> one element of a vector can't refer to another, we don't need the ctx->ctor
>> handling.  Perhaps we should handle vectors like we do PMF types in
>> cxx_eval_bare_aggregate?
>
> That may be abstractly better, but we do currently wrap constructors in
> target_exprs for vector compound_literals (which is what I was
> following).  See the get_target_expr_sfinae  calls in
> finish_compound_literal for instance.  That happens for  the '(v4si){(0,
> 0)}' subexpression of the testcase.

Sure, but that also seems unnecessary; vector rvalues don't have object 
identity the way class and array rvalues do.

Jason

Re: [C++ PATCH] PR 70501, ICE in verify ctor sanity

[Nathan Sidwell]

On 04/06/16 07:49, Jason Merrill wrote:
> On 04/05/2016 05:21 PM, Nathan Sidwell wrote:
>> On 04/05/16 12:40, Jason Merrill wrote:
>>
>>> It's not clear to me that we really need a TARGET_EXPR for vector values.  Since
>>> one element of a vector can't refer to another, we don't need the ctx->ctor
>>> handling.  Perhaps we should handle vectors like we do PMF types in
>>> cxx_eval_bare_aggregate?
>>
>> That may be abstractly better, but we do currently wrap constructors in
>> target_exprs for vector compound_literals (which is what I was
>> following).  See the get_target_expr_sfinae  calls in
>> finish_compound_literal for instance.  That happens for  the '(v4si){(0,
>> 0)}' subexpression of the testcase.
>
> Sure, but that also seems unnecessary; vector rvalues don't have object identity
> the way class and array rvalues do.

I'll investigate further.  At least we have a fallback now.

nathan

Re: [C++ PATCH] PR 70501, ICE in verify ctor sanity

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On 04/06/16 07:49, Jason Merrill wrote:

> Sure, but that also seems unnecessary; vector rvalues don't have object identity
> the way class and array rvalues do.

I attach 2 patches.

70501-2.patch fixes the ICE by treating VECTOR_TYPEs thesame as PMFs in 
cxx_eval_bare_aggregate).

70501-other.patch stops finish_compound_literal wrapping VECTOR_TYPEs in a 
TARGET_EXPR.  And also moves the comments  around, as I found them a little 
confusing.  We might want to wait until 7.0 to apply that patch, as it's not a 
regression.

For avoidance of doubt I tested the first patch both with and without the second 
patch.

ok?

nathan


[-- Attachment #2: 70501-2.patch --]
[-- Type: text/x-patch, Size: 1326 bytes --]

2016-04-06  Nathan Sidwell  <nathan@acm.org>

	PR c++/70501
	* constexpr.c (cxx_eval_bare_aggregate): Handle VECTOR_TYPE
	similarly to PMF.

	* g++.dg/init/pr70501.C: New.

Index: cp/constexpr.c
===================================================================
--- cp/constexpr.c	(revision 234768)
+++ cp/constexpr.c	(working copy)
@@ -2370,10 +2370,10 @@ cxx_eval_bare_aggregate (const constexpr
   tree type = TREE_TYPE (t);
 
   constexpr_ctx new_ctx;
-  if (TYPE_PTRMEMFUNC_P (type))
+  if (TYPE_PTRMEMFUNC_P (type) || VECTOR_TYPE_P (type))
     {
-      /* We don't really need the ctx->ctor business for a PMF, but it's
-	 simpler to use the same code.  */
+      /* We don't really need the ctx->ctor business for a PMF or
+	 vector, but it's simpler to use the same code.  */
       new_ctx = *ctx;
       new_ctx.ctor = build_constructor (type, NULL);
       new_ctx.object = NULL_TREE;
Index: testsuite/g++.dg/init/pr70501.C
===================================================================
--- testsuite/g++.dg/init/pr70501.C	(nonexistent)
+++ testsuite/g++.dg/init/pr70501.C	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-options "" } Not pedantic */
+
+typedef int v4si __attribute__ ((vector_size (16)));
+
+struct S { v4si v; };
+
+void
+fn2 (int i, int j)
+{
+  struct S s = { .v = i <= j + (v4si){(1, 2)} };
+}

[-- Attachment #3: 70501-other.patch --]
[-- Type: text/x-patch, Size: 1420 bytes --]

2016-04-06  Nathan Sidwell  <nathan@acm.org>

	* semantics.c (finish_compound_lteral): Don't wrap VECTOR_TYPEs in a
	TARGET_EXPR.

Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 234768)
+++ cp/semantics.c	(working copy)
@@ -2732,8 +2732,8 @@ finish_compound_literal (tree type, tree
   compound_literal = digest_init (type, compound_literal, complain);
   if (TREE_CODE (compound_literal) == CONSTRUCTOR)
     TREE_HAS_CONSTRUCTOR (compound_literal) = true;
-  /* Put static/constant array temporaries in static variables, but always
-     represent class temporaries with TARGET_EXPR so we elide copies.  */
+
+  /* Put static/constant array temporaries in static variables.  */
   if ((!at_function_scope_p () || CP_TYPE_CONST_P (type))
       && TREE_CODE (type) == ARRAY_TYPE
       && !TYPE_HAS_NONTRIVIAL_DESTRUCTOR (type)
@@ -2763,8 +2763,13 @@ finish_compound_literal (tree type, tree
 	return error_mark_node;
       return decl;
     }
-  else
-    return get_target_expr_sfinae (compound_literal, complain);
+
+  /* Represent other compound literals with TARGET_EXPR so we produce
+     an lvalue, but can elide copies.  */
+  if (!VECTOR_TYPE_P (type))
+    compound_literal = get_target_expr_sfinae (compound_literal, complain);
+
+  return compound_literal;
 }
 
 /* Return the declaration for the function-name variable indicated by

Re: [C++ PATCH] PR 70501, ICE in verify ctor sanity

[Jason Merrill]

On 04/07/2016 11:18 AM, Nathan Sidwell wrote:
> On 04/06/16 07:49, Jason Merrill wrote:
>
>> Sure, but that also seems unnecessary; vector rvalues don't have
>> object identity
>> the way class and array rvalues do.
>
> I attach 2 patches.
>
> 70501-2.patch fixes the ICE by treating VECTOR_TYPEs thesame as PMFs in
> cxx_eval_bare_aggregate).

OK.

> 70501-other.patch stops finish_compound_literal wrapping VECTOR_TYPEs in
> a TARGET_EXPR.  And also moves the comments  around, as I found them a
> little confusing.  We might want to wait until 7.0 to apply that patch,
> as it's not a regression.

Yes, OK once stage 1 reopens.

Jason

[committed] Add PR c++/70571 testcase

[Jakub Jelinek]

Hi!

On Thu, Apr 07, 2016 at 08:18:28AM -0700, Nathan Sidwell wrote:
> 2016-04-06  Nathan Sidwell  <nathan@acm.org>
> 
> 	PR c++/70501
> 	* constexpr.c (cxx_eval_bare_aggregate): Handle VECTOR_TYPE
> 	similarly to PMF.

This patch fixed also PR70571, so I've committed the testcase after
bootstrap/regtest on x86_64-linux and i686-linux to trunk and will close it
as a dup.

2016-04-12  Jakub Jelinek  <jakub@redhat.com>

	PR c++/70571
	* g++.dg/ext/pr70571.C: New test.

--- gcc/testsuite/g++.dg/ext/pr70571.C.jj	2016-04-12 19:28:38.378163234 +0200
+++ gcc/testsuite/g++.dg/ext/pr70571.C	2016-04-12 19:28:06.000000000 +0200
@@ -0,0 +1,10 @@
+// PR c++/70571
+// { dg-do compile }
+
+typedef int V __attribute__ ((vector_size (sizeof (int))));
+
+void
+foo (V *x, V *y, int z)
+{
+  *x = (z == *y);
+}


	Jakub