Re: [PATCH 2/2] OpenMP: Duplicate checking for map clauses in Fortran (PR107214)

[Tobias Burnus <tobias@codesourcery.com>]

Hi Julian:

On 07.12.22 20:13, Julian Brown wrote:
>> I know that this was the case before, but can you move the mark:1 etc.
>> after 'tlink'? In that case all bitfields are grouped together.
Thanks for doing so.
>> I wonder whether that also rejects the following – which seems to be
>> valid. The 'map' goes to 'target' and the 'firstprivate' to
>> 'parallel', cf. OpenMP 5.2, "17.2 Clauses on Combined and Composite
>> Constructs", [340:3-4 & 12-14]. (BTW: While some fixes went into 5.1
>> regarding this section, a likewise wording is already in 5.0.)
>>
>> (Testing showed: it give an ICE without the patch and an error with.)
> ...and this patch avoids the error for combined directives, and
> reorders the gfc_symbol bitfields.

All in all, I am fine with the patch - but I spotted some issues.

First, I think you need to set for some error cases mark = 0 to avoid duplicated errors.
Namely:

   ! Outputs the error twice ('Symbol ‘y’ present on multiple clauses')
   !$omp target has_device_addr(y) firstprivate(y)
   block; end block

  * * *

Additionally, I think it would be good to have besides 'target' + map/firstprivate (→ error)
also a testcase for 'target simd' + map/firstprivate → error

And I think also gives-no-error checks all combined 'target ...' that take firstprivate
should be added, cf. your own patch - possibly with looking at the original dump (scan-tree-dump)
to see that the clause is properly attached correctly. Example for 'target teams':

   !$omp target teams map(x) firstprivate(x)
   block; end block

(Works but no testcase.)

  * * *

The following is not diagnosed and gives an ICE:

!$omp target in_reduction(+: x) private(x)
   block; end block
end

The C testcase properly has:
   error: ‘x’ appears more than once in data-sharing clauses

Note: Using 'firstprivate' instead of 'private' shows the proper error also in Fortran.


The following does not ICE but does not make sense (and is rejected in C):

     4 | #pragma omp target private(x) map(x)

vs.

   !$omp target map(x) private(x)
   block; end block

(The latter produces "#pragma omp target private(x.0) map(tofrom:*x.0)", ups!)

  * * *

I also note that 'simd' accepts private such that

#pragma omp target simd private(x) map(x)
  for (int i=0; i < 0; i++)
  ;

!$omp target simd map(x) private(x)
do i = 1, 0; end do

is valid. (It is accepted by gcc and gfortran, i.e. it just needs to be added as testcase.)

  * * *

I note that C rejects {map(x),firstprivate(x)} + {has_device_addr(x),is_device_ptr(x)}',
but gfortran + your patch accepts:

   !$omp target map(x) has_device_addr(x)
   !$omp target map(x) is_device_ptr(x)

while

   !$omp target firstprivate(x) has_device_addr(x)
   !$omp target firstprivate(x) is_device_ptr(x)

is rejected – showing the error message twice.

Expected: I think it should show an error in all four cases - but only once.

> 2022-12-06  Julian Brown  <julian@codesourcery.com>
>
> gcc/fortran/
>          PR fortran/107214
>          * gfortran.h (gfc_symbol): Add data_mark, dev_mark, gen_mark and
>          reduc_mark bitfields.
>          * openmp.cc (resolve_omp_clauses): Use above bitfields to improve
>          duplicate clause detection.
>
> gcc/testsuite/
>          PR fortran/107214
>          * gfortran.dg/gomp/pr107214.f90: New test.

Thanks,

Tobias

-----------------
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