diff mbox

RFA (openmp): C++ PATCH to make some references TREE_CONSTANT

Message ID CADzB+2kF3vMzCEFibLW_S6LLvpNOMaS7k5YXOhPPGVL7L=gWcg@mail.gmail.com
State New
Headers show

Commit Message

Jason Merrill Nov. 15, 2016, 4:53 a.m. UTC 
The standard says that references that refer to a constant address can
be used in a constant-expression, but we haven't allowed that.  This
patch implements it, but without the parser.c hunk it broke
libgomp.c++/target-14.C.

Apparently the target mapping wants to use 't' in a way that doesn't
actually refer to x?  This seems like a strange use of references.
But if this is the desired behavior, does the parser.c patch seem like
a good way to prevent the compiler from replacing 't' with 'x' within
the openmp block?

Tested x86_64-pc-linux-gnu.
commit 93b5c38cb70b9c1e52a143415da76218583242d2
Author: Jason Merrill <jason@redhat.com>
Date:   Mon Nov 14 14:15:57 2016 -0500

            * decl2.c (decl_maybe_constant_var_p): References qualify.
            * init.c (constant_value_1): Always check decl_constant_var_p.
            * parser.c (cp_parser_omp_var_list_no_open): Clear
            DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P on variables in OpenMP
            clauses.

Comments

Jakub Jelinek Nov. 15, 2016, 9:32 a.m. UTC | #1 
On Mon, Nov 14, 2016 at 11:53:55PM -0500, Jason Merrill wrote:
> The standard says that references that refer to a constant address can
> be used in a constant-expression, but we haven't allowed that.  This
> patch implements it, but without the parser.c hunk it broke
> libgomp.c++/target-14.C.

The parser hunk can't be right, the decl you modify TREE_CONSTANT on
is not in any way "an OpenMP variable", it is just a variable used in some
OpenMP region, modifying the flag will affect all the handling of the
variable, both outside and inside of OpenMP regions.

> Apparently the target mapping wants to use 't' in a way that doesn't
> actually refer to x?  This seems like a strange use of references.

Yes.  The privatization and mapping of variables with reference type
in OpenMP generally works by privatizing or mapping what that reference
references, and then creating a new reference that references the privatized
or mapped variable.

Looking at the *.original dump diff without your patch to with your patch
minus the parser.c hunk shows:
-  if ((*a != 8 || *c != 142) || *t != 19)
+  if ((*a != 8 || *c != 142) || *(int &) &x != 19)
which might be fine outside of the OpenMP regions, from what you wrote I
understood is required in constexpr contexts, but can't be right in the
OpenMP regions - have to be deferred until omplower pass does whatever it
needs to do.  This is similar to DECL_VALUE_EXPR handling I've mentioned
yesterday, there it is also dangerous to just fold var with DECL_VALUE_EXPR
to its DECL_VALUE_EXPR until after omplower pass - the gimplifier gimplifies
such vars to their DECL_VALUE_EXPR only if it is ok (omp_notice_variable
function, e.g. together with disregard_value_expr langhook, decides on when
it is ok or not).

In target-14.C, there is mapping of reference t, so after lowering
there is going to be in the region used variable t' that refers to some int
in the target.  Your patch replaces uses of t in the region much earlier
with uses of &x, but that is something that isn't explicitly mapped, and
the OpenMP 4.5 rules say that such variable is firstprivatized implicitly,
so the code outside of the target region won't see any changes in the
variable.

So, is there a way to treat references the similarly?  I.e. only "fold"
reference vars to what they refer (DECL_INITIAL) in constexpr.c evaluation,
or gimplification where a langhook or omp_notice_variable etc. has the
last say on when it is ok to do that or not?

	Jakub
diff mbox

Patch

diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index 4ebc7dc..3952600 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -4127,7 +4127,7 @@  decl_constant_var_p (tree decl)
      constant until its initializer is complete in case it's used in
      its own initializer.  */
   maybe_instantiate_decl (decl);
-  return DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl);
+  return TREE_CONSTANT (decl);
 }
 
 /* Returns true if DECL could be a symbolic constant variable, depending on
@@ -4144,6 +4144,9 @@  decl_maybe_constant_var_p (tree decl)
   if (DECL_HAS_VALUE_EXPR_P (decl))
     /* A proxy isn't constant.  */
     return false;
+  if (TREE_CODE (type) == REFERENCE_TYPE)
+    /* References can be constant.  */
+    return true;
   return (CP_TYPE_CONST_NON_VOLATILE_P (type)
 	  && INTEGRAL_OR_ENUMERATION_TYPE_P (type));
 }
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index fe1f751..7bf07c3 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -1000,6 +1000,37 @@  dump_simple_decl (cxx_pretty_printer *pp, tree t, tree type, int flags)
     dump_type_suffix (pp, type, flags);
 }
 
+/* Print an IDENTIFIER_NODE that is the name of a declaration.  */
+
+static void
+dump_decl_name (cxx_pretty_printer *pp, tree t, int flags)
+{
+  /* These special cases are duplicated here so that other functions
+     can feed identifiers to error and get them demangled properly.  */
+  if (IDENTIFIER_TYPENAME_P (t))
+    {
+      pp_cxx_ws_string (pp, "operator");
+      /* Not exactly IDENTIFIER_TYPE_VALUE.  */
+      dump_type (pp, TREE_TYPE (t), flags);
+      return;
+    }
+  if (dguide_name_p (t))
+    {
+      dump_decl (pp, CLASSTYPE_TI_TEMPLATE (TREE_TYPE (t)),
+		 TFF_PLAIN_IDENTIFIER);
+      return;
+    }
+
+  const char *str = IDENTIFIER_POINTER (t);
+  if (!strncmp (str, "_ZGR", 3))
+    {
+      pp_cxx_ws_string (pp, "<temporary>");
+      return;
+    }
+
+  pp_cxx_tree_identifier (pp, t);
+}
+
 /* Dump a human readable string for the decl T under control of FLAGS.  */
 
 static void
@@ -1155,21 +1186,8 @@  dump_decl (cxx_pretty_printer *pp, tree t, int flags)
       gcc_unreachable ();
       break;
 
-      /* These special cases are duplicated here so that other functions
-	 can feed identifiers to error and get them demangled properly.  */
     case IDENTIFIER_NODE:
-      if (IDENTIFIER_TYPENAME_P (t))
-	{
-	  pp_cxx_ws_string (pp, "operator");
-	  /* Not exactly IDENTIFIER_TYPE_VALUE.  */
-	  dump_type (pp, TREE_TYPE (t), flags);
-	  break;
-	}
-      else if (dguide_name_p (t))
-	dump_decl (pp, CLASSTYPE_TI_TEMPLATE (TREE_TYPE (t)),
-		   TFF_PLAIN_IDENTIFIER);
-      else
-	pp_cxx_tree_identifier (pp, t);
+      dump_decl_name (pp, t, flags);
       break;
 
     case OVERLOAD:
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 1fad79c..b4b6cdb 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2078,10 +2078,9 @@  static tree
 constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p)
 {
   while (TREE_CODE (decl) == CONST_DECL
-	 || (strict_p
-	     ? decl_constant_var_p (decl)
-	     : (VAR_P (decl)
-		&& CP_TYPE_CONST_NON_VOLATILE_P (TREE_TYPE (decl)))))
+	 || decl_constant_var_p (decl)
+	 || (!strict_p && VAR_P (decl)
+	     && CP_TYPE_CONST_NON_VOLATILE_P (TREE_TYPE (decl))))
     {
       tree init;
       /* If DECL is a static data member in a template
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 9360ab0..8869c46 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -30743,6 +30743,9 @@  cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
 	    goto skip_comma;
 
 	  decl = cp_parser_lookup_name_simple (parser, name, token->location);
+	  if (VAR_P (decl))
+	    /* Make decl_constant_var_p false for OpenMP variables.  */
+	    TREE_CONSTANT (decl) = false;
 	  if (decl == error_mark_node)
 	    cp_parser_name_lookup_error (parser, name, decl, NLE_NULL,
 					 token->location);
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-ref10.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-ref10.C
new file mode 100644
index 0000000..925ac4e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-ref10.C
@@ -0,0 +1,5 @@ 
+// { dg-do compile { target c++11 } }
+
+int &&r = 42;
+static_assert (r, "");		// { dg-error "temporary" }
+// { dg-prune-output "assert" }
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-ref9.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-ref9.C
new file mode 100644
index 0000000..97f88fe
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-ref9.C
@@ -0,0 +1,9 @@ 
+// { dg-do compile { target c++11 } }
+
+int a[2] = { 1, 2 };
+
+int main()
+{
+  auto &r = a;
+  static_assert (&r[0] == &a[0], "");
+}