On Fri, Apr 08, 2016 at 07:35:35AM -0700, Cesar Philippidis wrote:
> The FEs a little inconsistent, and I didn't want to make this patch that
> invasive. Can the FE changes wait to gcc7?
Sure.
> 2016-04-08 Cesar Philippidis <ce...@codesourcery.com>
>
> PR lto/70289
> PR ipa/70348
> PR tree-optimization/70373
> PR middle-end/70533
> PR middle-end/70534
> PR middle-end/70535
>
No empty line between PR lines and * gimplify.c (... line.
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -7987,6 +7987,34 @@ gimplify_adjust_omp_clauses (gimple_seq *pre_p,
> gimple_seq body, tree *list_p,
> break;
> }
> decl = OMP_CLAUSE_DECL (c);
> + /* Data clasues associated with acc parallel reductions must be
> + compatible with present_or_copy. Warn and adjust the clause
> + if that is not the case. */
> + if (ctx->region_type == ORT_ACC_PARALLEL)
> + {
> + tree t = DECL_P (decl) ? decl : TREE_OPERAND (decl, 0);
> + n = NULL;
> +
> + if (DECL_P (t))
> + n = splay_tree_lookup (ctx->variables, (splay_tree_key)t);
There should be space before t.
> +
> + if (n && (n->value & GOVD_REDUCTION))
> + {
> + int kind = OMP_CLAUSE_MAP_KIND (c);
Use gomp_map_kind or enum gomp_map_kind instead of int?
> +
> + OMP_CLAUSE_MAP_IN_REDUCTION(c) = 1;
Space before (.
> + if ((kind & GOMP_MAP_TOFROM) != GOMP_MAP_TOFROM
> + && kind != GOMP_MAP_FORCE_PRESENT
> + && kind != GOMP_MAP_POINTER)
> + {
> + warning_at (OMP_CLAUSE_LOCATION (c), 0,
> + "incompatible data clause with reduction "
> + "on %qE; promoting to present_or_copy",
> + DECL_NAME (t));
> + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_TOFROM);
> + }
> + }
> + }
> if (!DECL_P (decl))
> {
> if ((ctx->region_type & ORT_TARGET) != 0
> @@ -8118,6 +8146,34 @@ gimplify_adjust_omp_clauses (gimple_seq *pre_p,
> gimple_seq body, tree *list_p,
>
> case OMP_CLAUSE_REDUCTION:
> decl = OMP_CLAUSE_DECL (c);
> + /* OpenACC reductions need a present_or_copy data clause.
> + Add one if necessary. Error is the reduction is private. */
> + if (ctx->region_type == ORT_ACC_PARALLEL)
> + {
> + n = splay_tree_lookup (ctx->variables, (splay_tree_key)decl);
Missing space.
> + if (n->value & (GOVD_PRIVATE | GOVD_FIRSTPRIVATE))
> + {
> + error_at (OMP_CLAUSE_LOCATION (c), "invalid private "
> + "reduction on %qE", DECL_NAME (decl));
> + }
Please avoid {}s around single statement. Better don't break the
message into multiple lines in this case, so
error_at (OMP_CLAUSE_LOCATION (c),
"invalid private reduction on %qE",
DECL_NAME (decl));
is more readable.
> + else if ((n->value & GOVD_MAP) == 0)
> + {
> + tree next = OMP_CLAUSE_CHAIN (c);
> + tree nc = build_omp_clause (UNKNOWN_LOCATION, OMP_CLAUSE_MAP);
Too long line, please wrap.
> + OMP_CLAUSE_SET_MAP_KIND (nc, GOMP_MAP_TOFROM);
> + OMP_CLAUSE_DECL (nc) = decl;
> + OMP_CLAUSE_CHAIN (c) = nc;
> + lang_hooks.decls.omp_finish_clause (nc, pre_p);
> + for (; nc; nc = OMP_CLAUSE_CHAIN (nc))
> + {
> + OMP_CLAUSE_MAP_IN_REDUCTION (nc) = 1;
> + if (OMP_CLAUSE_CHAIN (nc) == NULL)
> + break;
> + }
Then the nc; condition doesn't make sense. Perhaps then
while (1)
{
OMP_CLAUSE_MAP_IN_REDUCTION (nc) = 1;
if (OMP_CLAUSE_CHAIN (nc) == NULL)
break;
nc = OMP_CLAUSE_CHAIN (nc);
}
or
for (; ; nc = OMP_CLAUSE_CHAIN (nc))
{
OMP_CLAUSE_MAP_IN_REDUCTION (nc) = 1;
if (OMP_CLAUSE_CHAIN (nc) == NULL)
break;
}
?
> @@ -5624,22 +5625,38 @@ lower_oacc_reductions (location_t loc, tree clauses,
> tree level, bool inner,
> + else if ((OMP_CLAUSE_CODE (cls) == OMP_CLAUSE_FIRSTPRIVATE
> + || OMP_CLAUSE_CODE (cls) == OMP_CLAUSE_PRIVATE)
> + && orig == OMP_CLAUSE_DECL (cls))
> + {
> + is_private = true;
> + goto do_lookup;
> + }
Isn't this case rejected by the gimplifier?
> @@ -15829,7 +15874,10 @@ lower_omp_target (gimple_stmt_iterator *gsi_p,
> omp_context *ctx)
> if (!maybe_lookup_field (var, ctx))
> continue;
>
> - if (offloaded)
> + /* Don't remap oacc parallel reduction variables, because the
> + intermediate result must be local to each gang. */
> + if (offloaded && !(OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> + && OMP_CLAUSE_MAP_IN_REDUCTION(c)))
Missing space before after OMP_CLAUSE_MAP_IN_REDUCTION
Ok for trunk with those changes if the lower_oacc_reduction is_private
handling is still needed, if it is not needed, please clean that up.
Jakub
