OpenMP offloading vs. C++ static local variables

[Jakub Jelinek]

On Thu, Dec 07, 2023 at 04:09:04PM +0100, Thomas Schwinge wrote:
> > Yeah, I believe we should in the omp_discover_* sub-pass handle with
> > a help of a langhook automatically mark the guard variables (possibly
> > iff the guarded variable is marked?),
> 
> Looking at 'gcc/omp-offload.cc:omp_discover_implicit_declare_target' left
> me confused how that would be the code that marks up 'static' variables
> as implicit 'omp declare target'.  Working through a simple POD example
> (say, 's%static S s%static int i') it turns out, indeed that's not where
> that is happending, but instead 'gcc/gimplify.cc:gimplify_bind_expr' is
> the place:

Sure, that is for the case where those local statics should be marked
implicitly because they appear in a target function.
They can be also marked explicitly by the user through
#pragma omp declare target enter (name_of_static_var)
or
[[omp::decl (declare target)]] attribute on it etc.

> Now, the problem why that existing code doesn't trigger for C++ guard
> variables is that those are not in 'BIND_EXPR_VARS', due to C++ front end
> use of 'pushdecl_top_level_and_finish'.  If I change the C++ front end as
> follows (WIP; not reviewed in detail):
> 
>     --- gcc/cp/decl2.cc
>     +++ gcc/cp/decl2.cc
>     @@ -3576,5 +3576,6 @@ get_guard (tree decl)
>            DECL_IGNORED_P (guard) = 1;
>            TREE_USED (guard) = 1;
>     -      pushdecl_top_level_and_finish (guard, NULL_TREE);
>     +      pushdecl (guard);
>     +      cp_finish_decl (guard, NULL_TREE, false, NULL_TREE, 0);
>          }
>        return guard;

I don't think that is desirable.

> ..., then we do get the expected behavior:
> 
>     --- a-r.cc.006t.gimple        2023-12-07 13:27:36.254963406 +0100
>     +++ a-r.cc.006t.gimple        2023-12-07 14:10:39.352107107 +0100
>     @@ -5,6 +5,7 @@
>        bool retval.1;
>        bool D.2966;
>        static struct S s1;
>     +  static long long int _ZGVZL2f1vE2s1;
> 
>        gimple_call <__atomic_load_1, _1, &_ZGVZL2f1vE2s1, 2>
>        gimple_assign <eq_expr, retval.0, _1, 0, NULL>
> 
> ..., and offloading compilation works down to the expected next issue:
> 
>     ld: error: undefined symbol: __cxa_guard_acquire
>     >>> referenced by /tmp/ccAVyZpc.o:(f1())
>     [...]
>     collect2: error: ld returned 1 exit status
>     gcn mkoffload: fatal error: build-gcc/gcc/x86_64-pc-linux-gnu-accel-amdgcn-amdhsa-gcc returned 1 exit status
>     [...]
> 
> However: 'pushdecl_top_level_and_finish' has been used there "forever",
> and I currently have no clue at all whether changing that into 'pushdecl'
> would be acceptable, what effects that'd have elsewhere.

Exactly.

> That said...  Couldn't we indeed move this gimplification-level code re
> 'Static locals [...] need to be "omp declare target"' into
> 'gcc/omp-offload.cc:omp_discover_implicit_declare_target'?

The omp-offload.cc discovery stuff was added for stuff where the OpenMP
standard says something is implicitly declare target because there is
some use of it satisfying some rule.
Like, calls to functions defined in current compilation unit referenced in
target region or something similar, or such calls referenced in declare
target static var initializers.
So, that feels to me like the right spot to handle the guards as well.
Of course, the middle-end doesn't know about C++ FE's get_guard variable,
so it should be some new language hook which would take care of it.
The omp_discover_declare* functions can add further VAR_DECLs to the
worklist, so I'd probably call the new language hook in the
omp_discover_implicit_declare_target last loop.
Or maybe even better just handle that in the
cxx_omp_finish_decl_inits hook.  You can just
  FOR_EACH_VARIABLE (vnode)
    if (DECL_FUNCTION_SCOPE_P (vnode->decl)
	&& omp_declare_target_var_p (vnode->decl))
      {
	tree sname = mangle_guard_variable (decl);
	tree guard = get_global_binding (sname);
	if (guard)
	  ... mark guard as declare target if not yet marked ...
      }
because guard var initializers don't really mention anything and so
their addition doesn't need to trigger further worklist changes.

> > or e.g. rtti info (_ZTS*, _ZTI*)
> > and eventually figure out what we should do about virtual tables (_ZTV*).
> > The last case is most complicated, as it contains function pointers, and we
> > need to figure out if we mark all methods, or say replace some pointers in
> > the virtual table with NULLs or something that errors or terminates if it
> > isn't marked.
> 
> All those I plan to defer, for now.

Ok.

> > And sure, __cxa_guard_* would need to be implemented in the offloading
> > libsupc++.a or libstdc++.a.
> 
> Until proper libstdc++/libsupc++ support emerges (I'm working on it...),
> my idea was to add a temporary 'libgomp/config/accel/*.c' implementation
> (based on 'libstdc++-v3/libsupc++/guard.cc').

That looks reasonable.

	Jakub