PING Re: [PATCH] openmp: Implicit 'declare target' for C++ static initializers

[Kwok Cheung Yeung <kcy@codesourcery.com>]

Hello

This patch still needs review.

Thanks

Kwok

On 19/11/2020 6:07 pm, Kwok Cheung Yeung wrote:
> On 29/10/2020 10:03 am, Jakub Jelinek wrote:
>> I'm actually not sure how this can work correctly.
>> Let's say we have
>> int foo () { return 1; }
>> int bar () { return 2; }
>> int baz () { return 3; }
>> int qux () { return 4; }
>> int a = foo ();
>> int b = bar ();
>> int c = baz ();
>> int *d = &c;
>> int e = qux ();
>> int f = e + 1;
>> int *g = &f;
>> #pragma omp declare target to (b, d, g)
>> So, for the implicit declare target discovery, a is not declare target to,
>> nor is foo, and everything else is; b, d, g explicitly, c because it is
>> referenced in initializer of b, f because it is mentioned in initializer of
>> g and e because it is mentioned in initializer of f.
>> Haven't checked if the new function you've added is called before or after
>> analyze_function calls omp_discover_implicit_declare_target, but I don't
>> really see how it can work when it is not inside of that function, so that
>> discovery of new static vars that are implicitly declare target to doesn't
>> result in marking of its dynamic initializers too.  Perhaps we need a
>> langhook for that.  But if it is a separate function, either it is called
>> before the other discovery and will ignore static initializers for vars
>> that will only be marked as implicit declare target to later, or it is done
>> afterwards, but then it would really need to duplicate everything what the
>> other function does, otherwise it woiuldn't discover everything.
>>
> 
> I have added a new langhook GET_DECL_INIT that by default returns the 
> DECL_INITIAL of a variable declaration, but for C++ can also return the dynamic 
> initializer if present. omp_discover_implicit_declare_target and 
> omp_discover_declare_target_var_r have been changed to use the new langhook 
> instead of using DECL_INITIAL.
> 
> The dynamic initializer information is stored in a new variable 
> dynamic_initializers. The information is originally stored in static_aggregates, 
> but this is nulled by calling prune_vars_needing_no_initialization in 
> c_parse_final_cleanups. I copy the information into a separate variable before 
> it is discarded - this avoids any potential problems that may be caused by 
> trying to change the way that static_aggregates currently works.
> 
> With this, all the functions and variables in your example are marked correctly:
> 
> foo ()
> ...
> 
> __attribute__((omp declare target))
> bar ()
> ...
> 
> __attribute__((omp declare target))
> baz ()
> ...
> 
> __attribute__((omp declare target))
> qux ()
> ...
> 
> .offload_var_table:
>          .quad   g
>          .quad   8
>          .quad   d
>          .quad   8
>          .quad   b
>          .quad   4
>          .quad   c
>          .quad   4
>          .quad   f
>          .quad   4
>          .quad   e
>          .quad   4
> 
> Your example is now a compile test in g++.dg/gomp/.
> 
>> Anyway, that is one thing, the other is even if the implicit declare target
>> discovery handles those correctly, the question is what should we do
>> afterwards.  Because the C++ FE normally creates a single function that
>> performs the dynamic initialization of the TUs variables.  But that function
>> shouldn't be really declare target to, it initializes not only (explicit or
>> implicit) declare target to variables, but also host only variables.
>> So we'll probably need to create next to that host only TU constructor
>> also a device only constructor function that will only initialize the
>> declare target to variables.
> 
> Even without this patch, G++ currently accepts something like
> 
> int foo() { return 1; }
> int x = foo();
> #pragma omp declare target to(x)
> 
> but will not generate the device-side initializer for x, even though x is now 
> present on the device. So this part of the implementation is broken with or 
> without the patch.
> 
> Given that my patch doesn't make the current situation any worse, can I commit 
> this portion of it to trunk for now, and leave device-side dynamic 
> initialization for later?
> 
> Bootstrapped on x86_64 with no offloading, G++ testsuite ran with no 
> regressions, and no regressions in the libgomp testsuite with Nvidia offloading.
> 
> Thanks,
> 
> Kwok