diff mbox

RFC: C++/OMP PATCH for c++/53565 (libgomp for-2.C failure)

Message ID 4FE2CE88.2090104@redhat.com
State New
Headers show

Commit Message

Jason Merrill June 21, 2012, 7:34 a.m. UTC 
A recent change of mine caused auto to persist until instantiation time 
in more cases, and the OMP for instantiation code wasn't prepared to 
deal with that.  This patch simplifies the handling of the for-init-expr 
in an OMP loop by processing the DECL_EXPR right away, rather than 
trying to split handling it between tsubst_omp_for_iterator and tsubst_expr.

All the tests in the libgomp and gcc testsuites pass with this patch, 
but I'm not very confident about it because I don't fully understand 
what the code is doing.  In particular, I'm not sure what the condition 
controlling the code in tsubst_omp_for_iterator that looks at the 
clauses should be; init can never be a DECL_EXPR at that point anymore.

I'm also not sure whether moving the begin_omp_structured_block is 
necessary, but it seemed like a good idea since we're handling the 
DECL_EXPR earlier.

Does this make sense to you?

Comments

Jakub Jelinek June 25, 2012, 10:39 a.m. UTC | #1 
On Thu, Jun 21, 2012 at 12:34:32AM -0700, Jason Merrill wrote:
> All the tests in the libgomp and gcc testsuites pass with this
> patch, but I'm not very confident about it because I don't fully
> understand what the code is doing.  In particular, I'm not sure what
> the condition controlling the code in tsubst_omp_for_iterator that
> looks at the clauses should be; init can never be a DECL_EXPR at
> that point anymore.

Don't remember exactly, but I'd guess I wanted to preserve as much as
possible what the code was doing before for the non-class types.
After RECUR init can't be DECL_EXPR, right?  So if you do it unconditionally
it doesn't need to be handled.

> I'm also not sure whether moving the begin_omp_structured_block is
> necessary, but it seemed like a good idea since we're handling the
> DECL_EXPR earlier.

Looking at the code now, I wonder whether begin_omp_structured_block
shouldn't be moved later, not earlier, because the decl ctors should happen
before the body of the worksharing region.  But if it works as you wrote,
I'm fine with it, I think the testsuite coverage should catch problematic
cases.

	Jakub
Jason Merrill June 25, 2012, 3:04 p.m. UTC | #2 
On 06/25/2012 06:39 AM, Jakub Jelinek wrote:
> On Thu, Jun 21, 2012 at 12:34:32AM -0700, Jason Merrill wrote:
>> All the tests in the libgomp and gcc testsuites pass with this
>> patch, but I'm not very confident about it because I don't fully
>> understand what the code is doing.  In particular, I'm not sure what
>> the condition controlling the code in tsubst_omp_for_iterator that
>> looks at the clauses should be; init can never be a DECL_EXPR at
>> that point anymore.
>
> Don't remember exactly, but I'd guess I wanted to preserve as much as
> possible what the code was doing before for the non-class types.
> After RECUR init can't be DECL_EXPR, right?  So if you do it unconditionally
> it doesn't need to be handled.

Right, I just wasn't sure what the point of the test was.  Is it that if 
we're declaring the iteration variable in the for-init-statement, we 
don't need to worry about making it OMP private?

>> I'm also not sure whether moving the begin_omp_structured_block is
>> necessary, but it seemed like a good idea since we're handling the
>> DECL_EXPR earlier.
>
> Looking at the code now, I wonder whether begin_omp_structured_block
> shouldn't be moved later, not earlier, because the decl ctors should happen
> before the body of the worksharing region.

Should they?  It seems that each of the private copies of the decl gets 
initialized separately, which I would expect to be part of the 
worksharing region.

Jason
Jakub Jelinek June 25, 2012, 3:15 p.m. UTC | #3 
On Mon, Jun 25, 2012 at 11:04:42AM -0400, Jason Merrill wrote:
> Right, I just wasn't sure what the point of the test was.  Is it
> that if we're declaring the iteration variable in the
> for-init-statement, we don't need to worry about making it OMP
> private?

It should be made private always.  Even for
int
foo (void)
{
  int j;
  j = 6;
  #pragma omp for private(j)
  for (int i = 0; i < 6; i++)
    j = i;
  return j;
}
we declare it before #pragma omp for and make it private(i).

> >Looking at the code now, I wonder whether begin_omp_structured_block
> >shouldn't be moved later, not earlier, because the decl ctors should happen
> >before the body of the worksharing region.
> 
> Should they?  It seems that each of the private copies of the decl
> gets initialized separately, which I would expect to be part of the
> worksharing region.

Well, this is a worksharing region, so is already entered by all the
threads, thus the initialization is performed by all threads anyway, the
thing is just where the remapping of the var into the private var happens
(and that should be performed by the gimplifier).

Anyway, please go ahead with your patch.

	Jakub
diff mbox

Patch

commit a31dd0fa43b7d0d75e18b566501c3dad6f477e2b
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Jun 20 19:16:01 2012 -0700

    	PR c++/53565
    	* pt.c (tsubst_omp_for_iterator): Simplify DECL_EXPR handling.
    	(tsubst_expr) [OMP_FOR]: Here, too.

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 5e02c8c..cec244a 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -12659,22 +12659,23 @@  tsubst_omp_for_iterator (tree t, int i, tree declv, tree initv,
 #define RECUR(NODE)				\
   tsubst_expr ((NODE), args, complain, in_decl,	\
 	       integral_constant_expression_p)
-  tree decl, init, cond, incr, auto_node;
+  tree decl, init, cond, incr;
+  bool init_decl;
 
   init = TREE_VEC_ELT (OMP_FOR_INIT (t), i);
   gcc_assert (TREE_CODE (init) == MODIFY_EXPR);
-  decl = RECUR (TREE_OPERAND (init, 0));
+  decl = TREE_OPERAND (init, 0);
   init = TREE_OPERAND (init, 1);
-  auto_node = type_uses_auto (TREE_TYPE (decl));
-  if (auto_node && init)
+  init_decl = (init && TREE_CODE (init) == DECL_EXPR);
+  /* Do this before substituting into decl to handle 'auto'.  */
+  init = RECUR (init);
+  decl = RECUR (decl);
+  if (init_decl)
     {
-      tree init_expr = init;
-      if (TREE_CODE (init_expr) == DECL_EXPR)
-	init_expr = DECL_INITIAL (DECL_EXPR_DECL (init_expr));
-      init_expr = RECUR (init_expr);
-      TREE_TYPE (decl)
-	= do_auto_deduction (TREE_TYPE (decl), init_expr, auto_node);
+      gcc_assert (!processing_template_decl);
+      init = DECL_INITIAL (decl);
     }
+
   gcc_assert (!type_dependent_expression_p (decl));
 
   if (!CLASS_TYPE_P (TREE_TYPE (decl)))
@@ -13189,34 +13190,13 @@  tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
 	condv = make_tree_vec (TREE_VEC_LENGTH (OMP_FOR_INIT (t)));
 	incrv = make_tree_vec (TREE_VEC_LENGTH (OMP_FOR_INIT (t)));
 
+	stmt = begin_omp_structured_block ();
+
 	for (i = 0; i < TREE_VEC_LENGTH (OMP_FOR_INIT (t)); i++)
 	  tsubst_omp_for_iterator (t, i, declv, initv, condv, incrv,
 				   &clauses, args, complain, in_decl,
 				   integral_constant_expression_p);
 
-	stmt = begin_omp_structured_block ();
-
-	for (i = 0; i < TREE_VEC_LENGTH (initv); i++)
-	  if (TREE_VEC_ELT (initv, i) == NULL
-	      || TREE_CODE (TREE_VEC_ELT (initv, i)) != DECL_EXPR)
-	    TREE_VEC_ELT (initv, i) = RECUR (TREE_VEC_ELT (initv, i));
-	  else if (CLASS_TYPE_P (TREE_TYPE (TREE_VEC_ELT (initv, i))))
-	    {
-	      tree init = RECUR (TREE_VEC_ELT (initv, i));
-	      gcc_assert (init == TREE_VEC_ELT (declv, i));
-	      TREE_VEC_ELT (initv, i) = NULL_TREE;
-	    }
-	  else
-	    {
-	      tree decl_expr = TREE_VEC_ELT (initv, i);
-	      tree init = DECL_INITIAL (DECL_EXPR_DECL (decl_expr));
-	      gcc_assert (init != NULL);
-	      TREE_VEC_ELT (initv, i) = RECUR (init);
-	      DECL_INITIAL (DECL_EXPR_DECL (decl_expr)) = NULL;
-	      RECUR (decl_expr);
-	      DECL_INITIAL (DECL_EXPR_DECL (decl_expr)) = init;
-	    }
-
 	pre_body = push_stmt_list ();
 	RECUR (OMP_FOR_PRE_BODY (t));
 	pre_body = pop_stmt_list (pre_body);
diff --git a/gcc/testsuite/g++.dg/gomp/for-19.C b/gcc/testsuite/g++.dg/gomp/for-19.C
index 7c56719..4441a29 100644
--- a/gcc/testsuite/g++.dg/gomp/for-19.C
+++ b/gcc/testsuite/g++.dg/gomp/for-19.C
@@ -26,8 +26,8 @@  template <typename T>
 void
 f3 (void)
 {
-#pragma omp for		// { dg-error "forbids incrementing a pointer of type" }
-  for (T q = T (p); q < T (p + 4); q++)
+#pragma omp for
+  for (T q = T (p); q < T (p + 4); q++)	// { dg-error "forbids incrementing a pointer of type" }
     ;
 }