Re: [Patch] OpenMP: Fix use_device_{addr,ptr} with in-data-sharing arg
On Wed, Apr 20, 2022 at 03:19:38PM +0200, Tobias Burnus wrote:
> For array-descriptor vars, the descriptor is assigned to a temporary. However,
> this failed when the clause's argument was in turn in a data-sharing clause
> as the outer context's VALUE_EXPR wasn't used.
>
> gcc/ChangeLog:
>
> * omp-low.cc (lower_omp_target): Fix use_device_{addr,ptr} with list
> item that is in an outer data-sharing clause.
>
> libgomp/ChangeLog:
>
> * testsuite/libgomp.fortran/use_device_addr-5.f90: New test.
>
> gcc/omp-low.cc | 22 ++--
> .../libgomp.fortran/use_device_addr-5.f90 | 143
> +
> 2 files changed, 156 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc
> index bf5779b6543..6e387fd9a61 100644
> --- a/gcc/omp-low.cc
> +++ b/gcc/omp-low.cc
> @@ -13656,26 +13656,30 @@ lower_omp_target (gimple_stmt_iterator *gsi_p,
> omp_context *ctx)
> new_var = lookup_decl (var, ctx);
> new_var = DECL_VALUE_EXPR (new_var);
> tree v = new_var;
> + tree v2 = var;
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_USE_DEVICE_PTR
> + || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_USE_DEVICE_ADDR)
> + {
> + v2 = maybe_lookup_decl_in_outer_ctx (var, ctx);
> + if (DECL_HAS_VALUE_EXPR_P (v2))
> + v2 = DECL_VALUE_EXPR (v2);
I don't understand the above 2 lines, why do you need that?
Regardless whether v2 has DECL_VALUE_EXPR or not, the type of the
DECL_VALUE_EXPR (v2) and v2 should be the same, build_fold_indirect_ref
should work on both and then v2 is only used as second operand of
gimplify_assign, where the gimplifier makes sure to handle DECL_VALUE_EXPR
correctly. I certainly don't see any difference in the *.omplower dump
if I comment out the above 2 lines.
Otherwise LGTM, so if the 2 lines aren't needed, please also drop the
{}s around v2 = maybe_lookup_decl_in_outer_ctx (var, ctx); and reindent.
> + }
>
> if (is_ref)
> {
> - var = build_fold_indirect_ref (var);
> - gimplify_expr (&var, &assign_body, NULL, is_gimple_val,
> -fb_rvalue);
> - v = create_tmp_var_raw (TREE_TYPE (var), get_name (var));
> + v2 = build_fold_indirect_ref (v2);
> + v = create_tmp_var_raw (TREE_TYPE (v2), get_name (var));
> gimple_add_tmp_var (v);
> TREE_ADDRESSABLE (v) = 1;
> - gimple_seq_add_stmt (&assign_body,
> - gimple_build_assign (v, var));
> + gimplify_assign (v, v2, &assign_body);
> tree rhs = build_fold_addr_expr (v);
> gimple_seq_add_stmt (&assign_body,
>gimple_build_assign (new_var, rhs));
> }
> else
> - gimple_seq_add_stmt (&assign_body,
> -gimple_build_assign (new_var, var));
> + gimplify_assign (new_var, v2, &assign_body);
>
> - tree v2 = lang_hooks.decls.omp_array_data (unshare_expr (v),
> false);
> + v2 = lang_hooks.decls.omp_array_data (unshare_expr (v), false);
> gcc_assert (v2);
> gimplify_expr (&x, &assign_body, NULL, is_gimple_val,
> fb_rvalue);
> gimple_seq_add_stmt (&assign_body,
Jakub
Re: [PATCH, stage 1] Fortran: Add support for OMP non-rectangular loops
On Fri, Mar 25, 2022 at 08:03:38PM -0600, Sandra Loosemore wrote: > This patch adds support for OMP 5.1 "canonical loop nest form" to the > Fortran front end, marks non-rectangular loops for processing > by the middle end, and implements missing checks in the gimplifier > for additional prohibitions on non-rectangular loops. > > Note that the OMP spec also prohibits non-rectangular loops with the TILE > construct; that construct hasn't been implemented yet, so that error will > need to be filled in later. > > gcc/fortran/ > * gfortran.h (struct gfc_omp_clauses): Add non_rectangular bit. > * openmp.cc (is_outer_iteration_variable): New function. > (expr_is_invariant): New function. > (bound_expr_is_canonical): New function. > (resolve_omp_do): Replace existing non-rectangularity error with > check for canonical form and setting non_rectangular bit. > * trans-openmp.cc (gfc_trans_omp_do): Transfer non_rectangular > flag to generated tree structure. > > gcc/ > * gimplify.cc (gimplify_omp_for): Update messages for SCHEDULED > and ORDERED clause conflict errors. Add check for GRAINSIZE and > NUM_TASKS on TASKLOOP. > > gcc/testsuite/ > * c-c++-common/gomp/loop-6.c (f3): New function to test TASKLOOP > diagnostics. > * gfortran.dg/gomp/collapse1.f90: Update expected messages. > * gfortran.dg/gomp/pr85313.f90: Remove dg-error on non-rectangular > loops that are now accepted. > * gfortran.dg/gomp/non-rectangular-loop.f90: New file. > * gfortran.dg/gomp/canonical-loop-1.f90: New file. > * gfortran.dg/gomp/canonical-loop-2.f90: New file. > > --- a/gcc/gimplify.cc > +++ b/gcc/gimplify.cc > @@ -12468,11 +12468,11 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p) > OMP_CLAUSE_SCHEDULE)) > error_at (EXPR_LOCATION (for_stmt), > "%qs clause may not appear on non-rectangular %qs", > - "schedule", "for"); > + "schedule", (lang_GNU_Fortran () ? "do" : "for")); > if (omp_find_clause (OMP_FOR_CLAUSES (for_stmt), OMP_CLAUSE_ORDERED)) > error_at (EXPR_LOCATION (for_stmt), > "%qs clause may not appear on non-rectangular %qs", > - "ordered", "for"); > + "ordered", (lang_GNU_Fortran () ? "do" : "for")); Please drop the superfluous ()s around the argument, just use "...", lang_GNU_Fortran () ? "do" : "for"); Ok for trunk with that nit fixed. And thanks for discovering the missing grainsize/num_tasks checks. Jakub
Re: [Patch] OpenMP: Fix use_device_{addr,ptr} with in-data-sharing arg
Hi Jakub,
On 04.05.22 14:03, Jakub Jelinek wrote:
On Wed, Apr 20, 2022 at 03:19:38PM +0200, Tobias Burnus wrote:
--- a/gcc/omp-low.cc
+++ b/gcc/omp-low.cc
@@ -13656,26 +13656,30 @@ lower_omp_target (gimple_stmt_iterator *gsi_p,
omp_context *ctx)
new_var = lookup_decl (var, ctx);
new_var = DECL_VALUE_EXPR (new_var);
tree v = new_var;
+tree v2 = var;
+if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_USE_DEVICE_PTR
+|| OMP_CLAUSE_CODE (c) == OMP_CLAUSE_USE_DEVICE_ADDR)
+ {
+v2 = maybe_lookup_decl_in_outer_ctx (var, ctx);
+if (DECL_HAS_VALUE_EXPR_P (v2))
+ v2 = DECL_VALUE_EXPR (v2);
I don't understand the above 2 lines, why do you need that?
I think it was intermittently required with some (half-)working patch.
But I concur that it is no longer is needed.
Otherwise LGTM, so if the 2 lines aren't needed, please also drop the
{}s around v2 = maybe_lookup_decl_in_outer_ctx (var, ctx); and reindent.
I did so, tested it also on my end and committed it as
r13-116-g3f8c389fe90bf565a6221a46bb7fb745dd4c1510
Thanks for the review!
Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
Re: [PATCH] Add a restriction on allocate clause (OpenMP 5.0)
On Fri, Feb 18, 2022 at 11:13:16PM +, Hafiz Abid Qadeer wrote: > An allocate clause in target region must specify an allocator > unless the compilation unit has requires construct with > dynamic_allocators clause. Current implementation of the allocate > clause did not check for this restriction. This patch fills that > gap. > > gcc/ChangeLog: > > * omp-low.cc (omp_maybe_offloaded_ctx): New prototype. > (scan_sharing_clauses): Check a restriction on allocate clause. > > gcc/testsuite/ChangeLog: > > * c-c++-common/gomp/allocate-2.c: Add tests. > * c-c++-common/gomp/allocate-8.c: New test. > * gfortran.dg/gomp/allocate-3.f90: Add tests. > * gcc.dg/gomp/pr104517.c: Update. I think it has even stronger requirement, that the allocator is present and is present in uses_allocators clause. But we don't parse uses_allocators, so this is a good step forward. Ok. Jakub
