[PATCH v4 6/7] OpenMP: Fortran front-end support for dispatch + adjust_args

[Paul-Antoine Arras]

Hi Tobias,

Replying to your last two messages here and attaching revised patches.

On 16/12/2024 22:34, Tobias Burnus wrote:
> I have not looked in depth at the patch, but managed to
> write C-ism code, which caused a segfault (due to a missing "call"),
> after gfortran issued a reasonable error. Can you fix it
> and, just to make sure, add it as another testcase?
> 
> foo.f90:18:7:
> 
>    18 |      g(a,b,c)
>       |       1
> Error: ‘g’ at (1) is not a variable
> foo.f90:20:3:
> 
>    20 | end
>       |   1
> Error: Unexpected END statement at (1)
> 
> Segmentation fault
> 
> * * *
> 
> The problem seems to be that during parsing,
> the location data is NULL, which the error diagnostic
> does not like:
> 
> (gdb) p gfc_current_locus
> $33 = {nextc = 0x0, u = {lb = 0x0, location = 0}}
> 
> (gdb) p gfc_at_eof()
> $36 = true
> 
> and st == ST_NONE.
> 
> I think the simplest is to check for the last one,
> and then return early. This will then print:
> 
> foo.f90:18:7:
> 
>    18 |      g(a,b,c)
>       |       1
> Error: ‘g’ at (1) is not a variable
> foo.f90:20:3:
> 
>    20 | end
>       |   1
> Error: Unexpected END statement at (1)
> f951: Error: Unexpected end of file in ‘foo.f90’
> 
> When the if st is ST_NONE then return check is added:
> 
> +static gfc_statement
> +parse_omp_dispatch (void)
> +{
> ...
> +  st = next_statement ();
> +  if (st == ST_NONE)
> +    return st;
> +  if (st == ST_CALL || st == ST_ASSIGNMENT)
> +    accept_statement (st);
> +  else

Fixed as suggested. Added testcase.

> * * *
> 
>>    Handling of `adjust_args` across translation units is missing due to PR115271.
> 
> Namely, https://gcc.gnu.org/PR115271 is about not storing 'declare variant' inside
> module files; when repeating the decl in an interface, it obviously works as
> 
> * * *
> 
> I think the patch is now okay, but I want to re-read it tomorrow - thus, please
> hold off for a couple of ours.
> 
> Possibly, others have comments as well :-)
> 
> * * *
> 
>>> TODO: We need to handle 'type(C), dimension(:)' - but I wonder
>>> whether that shouldn't be handled as part of 'use_device_addr'
>>> and we need to check whether the spec has to be updated.
>>>
>>> I filed the OpenMP lang-spec Issue #4443.
> ... and we eventually have to handle 'need_device_addr'/'has_device_addr', but those are follow-up topics.

Keeping an eye on the open issue.

On 17/12/2024 14:11, Tobias Burnus wrote:
> Additional comments: Can you hoist the condition out of the loop in:
> 
>> + for (gfc_omp_namelist *n = *head; n != NULL; n = n->next) + if 
>> (need_device_ptr_p) + n->u.need_device_ptr = true; 

Sure.

> * * *
> 
> I was about to complain that it didn't handle VALUE + OPTIONAL
> correctly, but that's a generic gfortran bug (or two):
>   ->https://gcc.gnu.org/PR118080
> 
> * * *
> 
> There is a bug - 'nowait' is not propagated. Trying:
> 
>    !$omp dispatch depend(inout:x) nowait
>      call g(a)
>    !$omp end dispatch
> 
> gives (-fdump-tree-gimple):      #pragma omp taskwait depend(inout:&x) nowait 
> but doing the equivalent !$omp dispatch depend(inout:x) call g(a) !$omp 
> end dispatch nowait gives: #pragma omp taskwait depend(inout:&x) i.e. 
> the 'nowait' got lost. * * *

Fixed and added testcase.

> Similar the original C code, which to my knowledge is now
> fixed + tested for, there is an issue related to handling nested
> function calls.
> 
> I think the attached testcase is fine, but it segfaults unless
> the default device is the initial device. The problem is that
> the pointer conversion also happens for the inner function but
> it should only do so for the outer one.
> 
> See attached testcase. – I think it can be seen by looking at the
> dump (and adding an -fdump-tree-gimple + scan test probably won't
> harm, as not everyone has a GPU and we might implement map as
> selfmap on APUs).

This is actually not specific to the Fortran FE. So I had to modify the 
middle end and the C++ parser as well. See attached pactches.

> Otherwise LGTM.
> 
> Tobias
> 

Thanks,
-- 
PA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-OpenMP-Fortran-front-end-support-for-dispatch-adjust.patch
Type: text/x-patch
Size: 80151 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/fortran/attachments/20241223/714e666d/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-C-fix.patch
Type: text/x-patch
Size: 1696 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/fortran/attachments/20241223/714e666d/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ME-fixes.patch
Type: text/x-patch
Size: 9663 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/fortran/attachments/20241223/714e666d/attachment-0005.bin>