Message ID | 4FE2CE88.2090104@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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
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" } ; }