From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7137 invoked by alias); 12 Mar 2008 10:12:02 -0000 Received: (qmail 7126 invoked by uid 22791); 12 Mar 2008 10:12:01 -0000 X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 12 Mar 2008 10:11:43 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id m2CABfB5001034 for ; Wed, 12 Mar 2008 06:11:42 -0400 Received: from devserv.devel.redhat.com (devserv.devel.redhat.com [10.10.36.72]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m2CABfs1012909 for ; Wed, 12 Mar 2008 06:11:41 -0400 Received: from devserv.devel.redhat.com (localhost.localdomain [127.0.0.1]) by devserv.devel.redhat.com (8.12.11.20060308/8.12.11) with ESMTP id m2CABfav023602 for ; Wed, 12 Mar 2008 06:11:41 -0400 Received: (from jakub@localhost) by devserv.devel.redhat.com (8.12.11.20060308/8.12.11/Submit) id m2CABfop023600 for gcc-patches@gcc.gnu.org; Wed, 12 Mar 2008 06:11:41 -0400 Date: Wed, 12 Mar 2008 10:12:00 -0000 From: Jakub Jelinek To: gcc-patches@gcc.gnu.org Subject: [PATCH] Don't use shared copy-in/out in nested omp parallels (PR middle-end/35549) Message-ID: <20080312101141.GS24887@devserv.devel.redhat.com> Reply-To: Jakub Jelinek Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4.1i X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2008-03/txt/msg00724.txt.bz2 Hi! To follow on the patch from yesterday, I've noticed that even for #pragma omp parallel there are cases when copy-in/out of a shared variable is invalid, see the testcase below. copy-in/out is possible only in the outermost parallel or, if the variable has been privatized somewhere from the outer parallel to the inner parallel. Regtested on x86_64-linux, committed to trunk/4.3. Will update gomp-3_0-branch now and rework the patch from yesterday. 2008-03-12 Jakub Jelinek PR middle-end/35549 * omp-low.c (maybe_lookup_decl): Constify first argument. (use_pointer_for_field): Change last argument from bool to omp_context *. Disallow shared copy-in/out in nested parallel if decl is shared in outer parallel too. (build_outer_var_ref, scan_sharing_clauses, lower_rec_input_clauses, lower_copyprivate_clauses, lower_send_clauses, lower_send_shared_vars): Adjust callers. * testsuite/libgomp.c/pr35549.c: New test. --- gcc/omp-low.c.jj 2008-02-19 11:11:11.000000000 +0100 +++ gcc/omp-low.c 2008-03-12 10:10:26.000000000 +0100 @@ -456,7 +456,7 @@ lookup_decl (tree var, omp_context *ctx) } static inline tree -maybe_lookup_decl (tree var, omp_context *ctx) +maybe_lookup_decl (const_tree var, omp_context *ctx) { tree *n; n = (tree *) pointer_map_contains (ctx->cb.decl_map, var); @@ -479,18 +479,18 @@ maybe_lookup_field (tree var, omp_contex return n ? (tree) n->value : NULL_TREE; } -/* Return true if DECL should be copied by pointer. SHARED_P is true - if DECL is to be shared. */ +/* Return true if DECL should be copied by pointer. SHARED_CTX is + the parallel context if DECL is to be shared. */ static bool -use_pointer_for_field (const_tree decl, bool shared_p) +use_pointer_for_field (const_tree decl, omp_context *shared_ctx) { if (AGGREGATE_TYPE_P (TREE_TYPE (decl))) return true; /* We can only use copy-in/copy-out semantics for shared variables when we know the value is not accessible from an outer scope. */ - if (shared_p) + if (shared_ctx) { /* ??? Trivially accessible from anywhere. But why would we even be passing an address in this case? Should we simply assert @@ -510,6 +510,34 @@ use_pointer_for_field (const_tree decl, address taken. */ if (TREE_ADDRESSABLE (decl)) return true; + + /* Disallow copy-in/out in nested parallel if + decl is shared in outer parallel, otherwise + each thread could store the shared variable + in its own copy-in location, making the + variable no longer really shared. */ + if (!TREE_READONLY (decl) && shared_ctx->is_nested) + { + omp_context *up; + + for (up = shared_ctx->outer; up; up = up->outer) + if (maybe_lookup_decl (decl, up)) + break; + + if (up && is_parallel_ctx (up)) + { + tree c; + + for (c = OMP_PARALLEL_CLAUSES (up->stmt); + c; c = OMP_CLAUSE_CHAIN (c)) + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_SHARED + && OMP_CLAUSE_DECL (c) == decl) + break; + + if (c) + return true; + } + } } return false; @@ -596,7 +624,7 @@ build_outer_var_ref (tree var, omp_conte } else if (is_parallel_ctx (ctx)) { - bool by_ref = use_pointer_for_field (var, false); + bool by_ref = use_pointer_for_field (var, NULL); x = build_receiver_ref (var, by_ref, ctx); } else if (ctx->outer) @@ -966,7 +994,7 @@ scan_sharing_clauses (tree clauses, omp_ gcc_assert (is_parallel_ctx (ctx)); decl = OMP_CLAUSE_DECL (c); gcc_assert (!is_variable_sized (decl)); - by_ref = use_pointer_for_field (decl, true); + by_ref = use_pointer_for_field (decl, ctx); /* Global variables don't need to be copied, the receiver side will use them directly. */ if (is_global_var (maybe_lookup_decl_in_outer_ctx (decl, ctx))) @@ -1001,7 +1029,7 @@ scan_sharing_clauses (tree clauses, omp_ && ! is_global_var (maybe_lookup_decl_in_outer_ctx (decl, ctx))) { - by_ref = use_pointer_for_field (decl, false); + by_ref = use_pointer_for_field (decl, NULL); install_var_field (decl, by_ref, ctx); } install_var_local (decl, ctx); @@ -1014,7 +1042,7 @@ scan_sharing_clauses (tree clauses, omp_ case OMP_CLAUSE_COPYIN: decl = OMP_CLAUSE_DECL (c); - by_ref = use_pointer_for_field (decl, false); + by_ref = use_pointer_for_field (decl, NULL); install_var_field (decl, by_ref, ctx); break; @@ -1751,7 +1779,7 @@ lower_rec_input_clauses (tree clauses, t /* Set up the DECL_VALUE_EXPR for shared variables now. This needs to be delayed until after fixup_child_record_type so that we get the correct type during the dereference. */ - by_ref = use_pointer_for_field (var, true); + by_ref = use_pointer_for_field (var, ctx); x = build_receiver_ref (var, by_ref, ctx); SET_DECL_VALUE_EXPR (new_var, x); DECL_HAS_VALUE_EXPR_P (new_var) = 1; @@ -1794,7 +1822,7 @@ lower_rec_input_clauses (tree clauses, t break; case OMP_CLAUSE_COPYIN: - by_ref = use_pointer_for_field (var, false); + by_ref = use_pointer_for_field (var, NULL); x = build_receiver_ref (var, by_ref, ctx); x = lang_hooks.decls.omp_clause_assign_op (c, new_var, x); append_to_statement_list (x, ©in_seq); @@ -2007,7 +2035,7 @@ lower_copyprivate_clauses (tree clauses, continue; var = OMP_CLAUSE_DECL (c); - by_ref = use_pointer_for_field (var, false); + by_ref = use_pointer_for_field (var, NULL); ref = build_sender_ref (var, ctx); x = lookup_decl_in_outer_ctx (var, ctx); @@ -2059,7 +2087,7 @@ lower_send_clauses (tree clauses, tree * continue; if (is_variable_sized (val)) continue; - by_ref = use_pointer_for_field (val, false); + by_ref = use_pointer_for_field (val, NULL); switch (OMP_CLAUSE_CODE (c)) { @@ -2129,7 +2157,7 @@ lower_send_shared_vars (tree *ilist, tre mapping for OVAR. */ var = lookup_decl_in_outer_ctx (ovar, ctx); - if (use_pointer_for_field (ovar, true)) + if (use_pointer_for_field (ovar, ctx)) { x = build_sender_ref (ovar, ctx); var = build_fold_addr_expr (var); --- libgomp/testsuite/libgomp.c/pr35549.c.jj 2008-03-12 10:23:34.000000000 +0100 +++ libgomp/testsuite/libgomp.c/pr35549.c 2008-03-12 10:23:08.000000000 +0100 @@ -0,0 +1,30 @@ +/* PR middle-end/35549 */ +/* { dg-do run } */ + +#include +#include + +int +main (void) +{ + int i = 6, n = 0; + omp_set_dynamic (0); + omp_set_nested (1); + #pragma omp parallel shared (i) num_threads (3) + { + if (omp_get_num_threads () != 3) + #pragma omp atomic + n += 1; + #pragma omp parallel shared (i) num_threads (4) + { + if (omp_get_num_threads () != 4) + #pragma omp atomic + n += 1; + #pragma omp critical + i += 1; + } + } + if (n == 0 && i != 6 + 3 * 4) + abort (); + return 0; +} Jakub