Hi Julian!
Two more questions here, in context of <https://gcc.gnu.org/PR102330>
"[12 Regression] ICE in expand_gimple_stmt_1, at cfgexpand.c:3932 since
r12-980-g29a2f51806c":
On 2019-06-03T17:02:45+0100, Julian Brown <jul...@codesourcery.com> wrote:
> This is a new version of the patch, rebased
The code as we've now got it in master branch has changed some more, but
I think the behavior I'm seeing may have been introduced here:
> and with a couple of
> additional bugfixes, as follows:
>
> Firstly, in mark_oacc_gangprivate, each decl is looked up (using
> maybe_lookup_decl) to apply the "oacc gangprivate" attribute to the
> innermost-nested copy of the decl.
> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -137,6 +137,12 @@ struct omp_context
> + /* Addressable variable decls in this context. */
> + vec<tree> *oacc_addressable_var_decls;
> };
> +/* Record vars listed in private clauses in CLAUSES in CTX. This information
> + is used to mark up variables that should be made private per-gang. */
> +
> +static void
> +oacc_record_private_var_clauses (omp_context *ctx, tree clauses)
> +{
> + tree c;
> +
> + if (!ctx)
> + return;
> +
> + for (c = clauses; c; c = OMP_CLAUSE_CHAIN (c))
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_PRIVATE)
> + {
> + tree decl = OMP_CLAUSE_DECL (c);
> + if (VAR_P (decl) && TREE_ADDRESSABLE (decl))
> + ctx->oacc_addressable_var_decls->safe_push (decl);
> + }
> +}
So, here we analyze 'OMP_CLAUSE_DECL (c)' (as is, without translation
through 'lookup_decl (decl, ctx)')...
> +/* Record addressable vars declared in BINDVARS in CTX. This information is
> + used to mark up variables that should be made private per-gang. */
> +
> +static void
> +oacc_record_vars_in_bind (omp_context *ctx, tree bindvars)
> +{
> + if (!ctx)
> + return;
> +
> + for (tree v = bindvars; v; v = DECL_CHAIN (v))
> + if (VAR_P (v) && TREE_ADDRESSABLE (v))
> + ctx->oacc_addressable_var_decls->safe_push (v);
> +}
..., and similarly here analyze 'v' (without 'lookup_decl (v, ctx)')...
> +/* Mark addressable variables which are declared implicitly or explicitly as
> + gang private with a special attribute. These may need to have their
> + declarations altered later on in compilation (e.g. in
> + execute_oacc_device_lower or the backend, depending on how the OpenACC
> + execution model is implemented on a given target) to ensure that sharing
> + semantics are correct. */
> +
> +static void
> +mark_oacc_gangprivate (vec<tree> *decls, omp_context *ctx)
> +{
> + int i;
> + tree decl;
> +
> + FOR_EACH_VEC_ELT (*decls, i, decl)
> + {
> + for (omp_context *thisctx = ctx; thisctx; thisctx = thisctx->outer)
> + {
> + tree inner_decl = maybe_lookup_decl (decl, thisctx);
> + if (inner_decl)
> + {
> + decl = inner_decl;
> + break;
> + }
> + }
> + if (!lookup_attribute ("oacc gangprivate", DECL_ATTRIBUTES (decl)))
> + {
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fprintf (dump_file,
> + "Setting 'oacc gangprivate' attribute for decl:");
> + print_generic_decl (dump_file, decl, TDF_SLIM);
> + fputc ('\n', dump_file);
> + }
> + DECL_ATTRIBUTES (decl)
> + = tree_cons (get_identifier ("oacc gangprivate"),
> + NULL, DECL_ATTRIBUTES (decl));
> + }
> + }
> +}
..., but here we action on the 'maybe_lookup_decl'-translated
'inner_decl', if applicable. In certain cases that one may be different
from the original 'decl'. (In particular (only?), when the OMP lowering
has made 'decl' "late 'TREE_ADDRESSABLE'".) This assymetry I understand
to give rise to <https://gcc.gnu.org/PR102330> "[12 Regression] ICE in
expand_gimple_stmt_1, at cfgexpand.c:3932 since r12-980-g29a2f51806c".
It makes sense to me that we do the OpenACC privatization on the
'lookup_decl' -- but shouldn't we then do that in the analysis phase,
too? (This appears to work fine for OpenACC 'private' clauses (..., and
avoids marking a few as addressable/gang-private), and for those in
'gimple_bind_vars' it doesn't seem to make a difference (for the current
test cases and/or compiler transformations).)
And, second question: what case did you run into or foresee, that you
here need the 'thisctx' loop and 'maybe_lookup_decl', instead of a plain
'lookup_decl (decl, ctx)'? Per my testing that's sufficient.
Unless you think this needs more consideration, I suggest to do these two
changes. (I have a WIP patch in testing.)
Grüße
Thomas
-----------------
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
