Re: [PATCH] openmp: Add support for the 'indirect' clause in C/C++

[Thomas Schwinge <thomas@codesourcery.com>]

Hi!

On 2023-11-09T17:00:11+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> On 09.11.23 13:24, Thomas Schwinge wrote:
>> Also, assuming that the order of appearance of 'IND_FUNC_MAP' does matter
>> as it does for 'FUNC_MAP', ... https://github.com/MentorEmbedded/nvptx-tools/pull/29 ...
>
> It should matter. Thus, we should indeed update nvptx-tools for this!
>
> For hello-world it probably does not show up that easily as there are
> only very few such tagged functions. But especially once it gets used
> for C++ virtual functions, the number of function will be that large
> that the ordering issue is likely to occur in the real world.
>
> (I shouldn't have missed this – given that I debugging and reported the
> original issue.)

Let's blame this on the inadequate general (non-)handling of these
directives in nvptx-tools 'as' -- which, as I said, I'll address once
Kwok has fixed this specific issue (with test case, please).
(Generalize/refactor after fixing specific issue.)

> (BTW: OMP_CLAUSE_INDIRECT is only used intermittendly in the C/C++ FEs
> and not in the ME as it is soon turned into an attribute string.)

OK, that does explain:

>> I would've assumed handling for 'OMP_CLAUSE_INDIRECT' to also be
>> necessary in the following places:
>>
>>    - 'gcc/c-family/c-omp.cc:c_omp_split_clauses'
> "split_clauses" applies only to combined composite constructs like
> 'target'+'parallel' +'for' + 'simd' where clauses have to be added to
> the right constituent clause(s). Declarative directives cannot be combined.
>>    - 'gcc/cp/pt.cc:tsubst_omp_clauses',
>>    - 'gcc/gimplify.cc:gimplify_scan_omp_clauses',
>>      'gcc/gimplify.cc:gimplify_adjust_omp_clauses'
>>    - 'gcc/omp-low.cc:scan_sharing_clauses' (twice)
>>    - 'gcc/tree-nested.cc:convert_nonlocal_omp_clauses',
>>      'gcc/tree-nested.cc:convert_local_omp_clauses'
>>    - 'gcc/tree-pretty-print.cc:dump_omp_clause'

... this.

> Most of those seem to relate to executable directives

(That remark I don't understand.)

> – and not to
> declarative ones, where we attach DECL_ATTRIBUTES to a decl and process
> them. For functions, the pretty printer prints the attributes.

> Here, we use "omp declare target indirect" as attribute.

ACK.

> We use noclone,noinline attributes for 'declare target', thus, there
> should be no issue on this side and regarding tsubst_omp_clauses, as the
> clause is either present or not (either bare or with a parse-time
> constant logical), there is not much post processing needed.

That's not obvious to the casual reader of GCC source code, though.

> Thus, I bet that there is nothing to do for those.
>
>> Please verify, and add handling as well as test cases as necessary, or,
>> as applicable, put 'case OMP_CLAUSE_INDIRECT:' next to
>> 'default: gcc_unreachable ();' etc., if indeed that clause is not
>> expected there.
>
> What's the point of having it next to default if it is gcc_unreachable?

Instead of "bet", I suggest to document intentions: so that it's clear
that 'OMP_CLAUSE_INDIRECT' is not meant to be seen here vs. an accidental
omission.

> I mean there are several others which shouldn't be needed like
> OMP_CLAUSE_DEVICE_TYPE which also does not show up at gcc/cp/pt.cc.

Quite possible.  :-) I certainly wouldn't object to "handling" those,
too.

Generally, in my opinion, we should usually see 'case's listed for all
clause codes where we 'switch' on them, for example.

>>> --- a/libgomp/config/gcn/team.c
>>> +++ b/libgomp/config/gcn/team.c
>>> @@ -45,6 +46,9 @@ gomp_gcn_enter_kernel (void)
>>>   {
>>>     int threadid = __builtin_gcn_dim_pos (1);
>>>
>> Shouldn't this:
>>
>>> +  /* Initialize indirect function support.  */
>>> +  build_indirect_map ();
>>> +
>> ... be called inside here:
>>
>>>     if (threadid == 0)
>>>       {
>> ..., so that it's only executed by one thread?
> (concur)
>> Also, for my understanding: why is 'build_indirect_map' done at kernel
>> invocation time (here) instead of at image load time?
>
> The splay_tree is generated on the device itself - and we currently do
> not start a kernel during GOMP_OFFLOAD_load_image. We could, the
> question is whether it makes sense. (Generating the splay_tree on the
> host for the device is a hassle and error prone as it needs to use
> device pointers at the end.)

Hmm.  It seems conceptually cleaner to me to set this up upfront, and
avoids potentially slowing down every device kernel invocation (at least
another function call, and 'gomp_mutex_lock' check).  Though, I agree
this may be "in the noise" with regards to all the other stuff going on
in 'gomp_gcn_enter_kernel' and elsewhere...

What I just realize, what's also unclear to me is how the current
implementation works with regards to several images getting loaded --
don't we then overwrite 'GOMP_INDIRECT_ADDR_MAP' instead of
(conceptually) appending to it?

In the general case, additional images may also get loaded during
execution.  We thus need proper locking of the shared data structure, uh?
Or, can we have separate on-device data structures per image?  (I've not
yet thought about that in detail.)

Relatedly then, when images are unloaded, we also need to remove stale
items from the table, and release resources (for example, the
'GOMP_OFFLOAD_alloc' for 'map_target_addr').

>>> +++ b/libgomp/testsuite/libgomp.c-c++-common/declare-target-indirect-2.c

Another thing regarding this test case: testing
'-foffload-options=amdgcn-amdhsa=-march=gfx900' offloading on our
amdnano4 system, I see:

    +PASS: libgomp.c/../libgomp.c-c++-common/declare-target-indirect-2.c execution test

..., but:

    +FAIL: libgomp.c++/../libgomp.c-c++-common/declare-target-indirect-2.c execution test

    Memory access fault by GPU node-2 (Agent handle: 0x21b0530) on address 0x401000. Reason: Page not present or supervisor privilege.

Re-running this manually a few times, I got:

    pass: 5
    fail (as above): 3
    hang: 1

Otherwise, that system appears to behave normally, and a reboot did not
cure this.


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