Hi!
On 2021-04-19T12:23:56+0100, Julian Brown <jul...@codesourcery.com> wrote:
> On Thu, 15 Apr 2021 19:26:54 +0200
> Thomas Schwinge <tho...@codesourcery.com> wrote:
>> On 2021-02-26T04:34:50-0800, Julian Brown <jul...@codesourcery.com>
>> wrote:
>> > Two new target hooks are introduced:
>> > TARGET_GOACC_ADJUST_PRIVATE_DECL and TARGET_GOACC_EXPAND_VAR_DECL.
>> > The first can tweak a variable declaration at oaccdevlow time, and
>> > the second at expand time. The first or both of these target hooks
>> > can be used by a given offload target, depending on its strategy
>> > for implementing private variables.
>>
>> ACK.
>>
>> So, currently we're only looking at making the gang-private level
>> work. Regarding that, we have two configurations: (1) for GCN
>> offloading, 'targetm.goacc.adjust_private_decl' does the work (in
>> particular, change 'TREE_TYPE' etc.) and there is no
>> 'targetm.goacc.expand_var_decl', and (2) for nvptx offloading,
>> 'targetm.goacc.adjust_private_decl' only sets a marker ('oacc
>> gangprivate' attribute) and then 'targetm.goacc.expand_var_decl' does
>> the work.
>>
>> Therefore I suggest we clarify the (currently) expected handling
>> similar to:
>>
>> --- gcc/omp-offload.c
>> +++ gcc/omp-offload.c
>> @@ -1854,6 +1854,19 @@ oacc_rewrite_var_decl (tree *tp, int
>> *walk_subtrees, void *data) return NULL_TREE;
>> }
>>
>> +static tree
>> +oacc_rewrite_var_decl_ (tree *tp, int *walk_subtrees, void *data)
>> +{
>> + tree t = oacc_rewrite_var_decl (tp, walk_subtrees, data);
>> + if (targetm.goacc.expand_var_decl)
>> + {
>> + walk_stmt_info *wi = (walk_stmt_info *) data;
>> + var_decl_rewrite_info *info = (var_decl_rewrite_info *) wi->info;
>> + gcc_assert (!info->modified);
>> + }
>> + return t;
>> +}
>
> Why the ugly _ tail on the function name!? I don't think that's a
> typical GNU coding standards thing, is it?
Heh, that was just to make the WIP prototype changes diff as small as
possible. ;-)
>> +
>> /* Return TRUE if CALL is a call to a builtin atomic/sync operation. */
>> static bool
>> @@ -2195,6 +2208,9 @@ execute_oacc_device_lower ()
>> COMPONENT_REFS, ARRAY_REFS and plain VAR_DECLs are also
>> rewritten to use the new decl, adjusting types of appropriate tree
>> nodes as necessary. */
>> + if (targetm.goacc.expand_var_decl)
>> + gcc_assert (adjusted_vars.is_empty ());
>
> If you like
I've pushed "[OpenACC privatization] Explain two different configurations
[PR90115]" to master branch in commit
21803fcaebeab36de0d7b6b8cf6abb9389f5e51f, see attached.
> -- or do something like
>
>> if (targetm.goacc.adjust_private_decl)
> && !adjusted_vars.is_empty ())
>
> perhaps.
That, too, additionally: I've pushed "[OpenACC privatization] Skip
processing if no work to be done [PR90115]" to master branch in commit
ad4612cb048b261f6834e9155e41e40e9252c80b, see attached.
>> {
>> FOR_ALL_BB_FN (bb, cfun)
>> @@ -2217,7 +2233,7 @@ execute_oacc_device_lower ()
>> memset (&wi, 0, sizeof (wi));
>> wi.info = &info;
>>
>> - walk_gimple_op (stmt, oacc_rewrite_var_decl, &wi);
>> + walk_gimple_op (stmt, oacc_rewrite_var_decl_, &wi);
>>
>> if (info.modified)
>> update_stmt (stmt);
>>
>> Or, in fact, 'if (targetm.goacc.expand_var_decl)', skip the
>> 'adjusted_vars' handling completely?
>
> For the current pair of implementations, sure. I don't think it's
> necessary to set that as a constraint for future targets though? I
> guess it doesn't matter much until such a target exists.
>
>> I do understand that eventually (in particular, for worker-private
>> level?), both 'targetm.goacc.adjust_private_decl' and
>> 'targetm.goacc.expand_var_decl' may need to do things, but that's
>> currently not meant to be addressed, and thus not fully worked out and
>> implemented, and thus untested. Hence, 'assert' what currently is
>> implemented/tested, only.
>
> If you like, no strong feelings from me on that.
>
>> (Given that eventual goal, that's probably sufficient motivation to
>> indeed add the 'adjusted_vars' handling in generic 'gcc/omp-offload.c'
>> instead of moving it into the GCN back end?)
>
> I'm not sure what moving it to the GCN back end would look like. I
> guess it's a question of keeping the right abstractions in the right
> place.
Right. I guess we'll figure that out once we have more than one back end
using the 'adjusted_vars' machinery.
>> > + 1. They can be recreated, making a pointer to the variable in the
>> > new
>> > + address space, or
>> > +
>> > + 2. The address of the variable in the new address space can be
>> > taken,
>> > + converted to the default (original) address space, and the result of
>> > + that conversion subsituted in place of the original ADDR_EXPR node.
>> > +
>> > + Which of these is done depends on the gimple statement being
>> > processed.
>> > + At present atomic operations and inline asms use (1), and everything
>> > else
>> > + uses (2). At least on AMD GCN, there are atomic operations that work
>> > + directly in the LDS address space.
>> > +
>> > + COMPONENT_REFS, ARRAY_REFS and plain VAR_DECLs are also rewritten to
>> > use
>> > + the new decl, adjusting types of appropriate tree nodes as
>> > necessary. */
>>
>> [TS] As I understand, this is only relevant for GCN offloading, but
>> not nvptx, and I'll trust that these two variants make sense from a
>> GCN point of view (which I cannot verify easily).
>
> The idea (hope) is that that's what's necessary "generically", though
> the only target using that support is GCN at present. I.e. it's not
> supposed to be GCN-specific, necessarily. Of course though, who knows
> what some other exotic target will need? (We don't want to be in the
> state where each target has to start completely from scratch for this
> sort of thing, if we can help it.)
>
>> > + if (targetm.goacc.adjust_private_decl)
>> > + {
>> > + FOR_ALL_BB_FN (bb, cfun)
>> > + for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
>> > + !gsi_end_p (gsi);
>> > + gsi_next (&gsi))
>> > + {
>> > + gimple *stmt = gsi_stmt (gsi);
>> > + walk_stmt_info wi;
>> > + var_decl_rewrite_info info;
>> > +
>> > + info.avoid_pointer_conversion
>> > + = (is_gimple_call (stmt)
>> > + && is_sync_builtin_call (as_a <gcall *> (stmt)))
>> > + || gimple_code (stmt) == GIMPLE_ASM;
>> > + info.stmt = stmt;
>> > + info.modified = false;
>> > + info.adjusted_vars = &adjusted_vars;
>> > +
>> > + memset (&wi, 0, sizeof (wi));
>> > + wi.info = &info;
>> > +
>> > + walk_gimple_op (stmt, oacc_rewrite_var_decl, &wi);
>> > +
>> > + if (info.modified)
>> > + update_stmt (stmt);
>> > + }
>> > + }
>> > +
>> > free_oacc_loop (loops);
>> >
>> > return 0;
>>
>> [TS] As disucssed above, maybe can completely skip the 'adjusted_vars'
>> rewriting for nvptx offloading?
>
> Yeah sure, if you like.
Grüße
Thomas
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
Thürauf
>From 21803fcaebeab36de0d7b6b8cf6abb9389f5e51f Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Thu, 20 May 2021 15:44:09 +0200
Subject: [PATCH 1/2] [OpenACC privatization] Explain two different
configurations [PR90115]
gcc/
PR middle-end/90115
* omp-offload.c (execute_oacc_device_lower): Explain.
---
gcc/omp-offload.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/gcc/omp-offload.c b/gcc/omp-offload.c
index 36bd2e44d81..336b48d5a3b 100644
--- a/gcc/omp-offload.c
+++ b/gcc/omp-offload.c
@@ -2206,6 +2206,26 @@ execute_oacc_device_lower ()
gsi_next (&gsi);
}
+ /* Regarding the OpenACC privatization level, we're currently only looking at
+ making the gang-private level work. Regarding that, we have the following
+ configurations:
+
+ - GCN offloading: 'targetm.goacc.adjust_private_decl' does the work (in
+ particular, change 'TREE_TYPE', etc.) and there is no
+ 'targetm.goacc.expand_var_decl'.
+
+ - nvptx offloading: 'targetm.goacc.adjust_private_decl' only sets a
+ marker and then 'targetm.goacc.expand_var_decl' does the work.
+
+ Eventually (in particular, for worker-private level?), both
+ 'targetm.goacc.adjust_private_decl' and 'targetm.goacc.expand_var_decl'
+ may need to do things, but that's currently not meant to be addressed, and
+ thus not fully worked out and implemented, and thus untested. Hence,
+ 'assert' what currently is implemented/tested, only. */
+
+ if (targetm.goacc.expand_var_decl)
+ gcc_assert (adjusted_vars.is_empty ());
+
/* Make adjustments to gang-private local variables if required by the
target, e.g. forcing them into a particular address space. Afterwards,
ADDR_EXPR nodes which have adjusted variables as their argument need to
--
2.30.2
>From ad4612cb048b261f6834e9155e41e40e9252c80b Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Thu, 20 May 2021 15:45:06 +0200
Subject: [PATCH 2/2] [OpenACC privatization] Skip processing if no work to be
done [PR90115]
gcc/
PR middle-end/90115
* omp-offload.c (execute_oacc_device_lower): Skip processing if no
work to be done.
---
gcc/omp-offload.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gcc/omp-offload.c b/gcc/omp-offload.c
index 336b48d5a3b..8bfb8b36cf0 100644
--- a/gcc/omp-offload.c
+++ b/gcc/omp-offload.c
@@ -2246,7 +2246,8 @@ execute_oacc_device_lower ()
COMPONENT_REFS, ARRAY_REFS and plain VAR_DECLs are also rewritten to use
the new decl, adjusting types of appropriate tree nodes as necessary. */
- if (targetm.goacc.adjust_private_decl)
+ if (targetm.goacc.adjust_private_decl
+ && !adjusted_vars.is_empty ())
{
FOR_ALL_BB_FN (bb, cfun)
for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
--
2.30.2
