[OpenACC,privatization] Analyze 'lookup_decl'-translated DECL [PR90115, PR102330, PR104774]

  Hi!

On 2022-02-15T13:40:09+0000, Julian Brown <julian@codesourcery.com> wrote:
> On Mon, 14 Feb 2022 16:56:35 +0100
> Thomas Schwinge <thomas@codesourcery.com> wrote:
>> 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 <julian@codesourcery.com> wrote:
>> > +/* 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)')...
>
> I think you're right that this one should be using lookup_decl, but...
>
>> > +/* 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)')...
>
> I'm not so sure about this one: if the variables are declared at a
> particular binding level, I think they have to be in the current OMP
> context (and thus shadow any definitions that might be present in the
> parent context)? Maybe that can be confirmed via an assertion.

Yes, I've added an 'gcc_checking_assert (lookup_decl (v, ctx) == v);'.

>> > +/* 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).)
>
> Yes, I think you're right.
>
>> 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.
>
> I'd probably misunderstood about lookup_decl walking up through parent
> contexts itself... oops.
>
>> Unless you think this needs more consideration, I suggest to do these
>> two changes.  (I have a WIP patch in testing.)
>
> Sounds good to me.

Thanks for your conceptual review.  Pushed to master branch
commit 7a5e036b61aa088e6b8564bc9383d37dfbb4801e "[OpenACC privatization]
Analyze 'lookup_decl'-translated DECL [PR90115, PR102330, PR104774]", see
attached.

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

Message ID	87mthym4g6.fsf@euler.schwinge.homeip.net
State	New
Headers	DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E4C2A3857C65 IronPort-SDR: cnlbn3oHrcu7YaN20Na3YNwXllCpwxuraPg8XHRjQnv/iIiXyhEtIaaPX1peYIvDQfgsBsvUyV 4RFPIf5uojsLsEBJtN9NUwuOEbrUIdF/bqlT+KOQxhmbnqxwxjB6dZ4yj9zJyGVi5U8vpGv2BA dKboSYojBLWwxTgGMjVhtmbMXJnbQcbVICR8g4vcJb7RSjoqDIkNmoZr5sX7cuC50qYe+DCpcc TuovLUnrBtqjwRVlF5KVa4L/vMcRi+4KZbB7UFTNbUF+h2Iinq2Ziy9Il2lMhzqY7LY5w4WEJp zF8= From: Thomas Schwinge <thomas@codesourcery.com> To: <gcc-patches@gcc.gnu.org>, Julian Brown <julian@codesourcery.com> Subject: [OpenACC privatization] Analyze 'lookup_decl'-translated DECL [PR90115, PR102330, PR104774] In-Reply-To: <20220215134009.7fa33fb1@squid.athome> References: <20180813172151.6bfcece3@squid.athome> <524d9017-e7f0-87a2-6a62-9b23abd65ac2@codesourcery.com> <94ac317d-5e4c-7738-4b22-b59d29dc114a@codesourcery.com> <20180815174637.7f7f9666@squid.athome> <1532490C-015A-4F89-8512-FD9751C7EEA4@gmail.com> <20180816164643.01e26476@squid.athome> <F50AA786-4108-4461-AC74-7FDAD7968F73@gmail.com> <20181211150811.47a032cf@squid.athome> <20190603170245.4a62a0ad@squid.athome> <87fsolsaq4.fsf@euler.schwinge.homeip.net> <20220215134009.7fa33fb1@squid.athome> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/27.1 (x86_64-pc-linux-gnu) Date: Thu, 10 Mar 2022 12:28:57 +0100 Message-ID: <87mthym4g6.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Precedence: list Cc: Jakub Jelinek <jakub@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org>
Series	[OpenACC,privatization] Analyze 'lookup_decl'-translated DECL [PR90115, PR102330, PR104774] \| [OpenACC,privatization] Analyze 'lookup_decl'-translated DECL [PR90115, PR102330, PR104774]

[OpenACC,privatization] Analyze 'lookup_decl'-translated DECL [PR90115, PR102330, PR104774]

Commit Message

Patch