On Thu, 8 Dec 2022 13:04:20 +0100
Tobias Burnus <tobias@codesourcery.com> wrote:

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

I believe this patch covers all the above cases (hopefully
appropriately generalised), at least for Fortran. I haven't attempted
to fix any missing cases for C, for now.

Re-tested with offloading to NVPTX (with a few supporting patches, as
before).

Does this look OK now?

Thanks,

Julian