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
