[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65835#1623939 , @hfinkel wrote:

> In D65835#1623903 , @ABataev wrote:
>
> > In D65835#1623883 , @hfinkel wrote:
> >
> > > In D65835#1623814 , @hfinkel 
> > > wrote:
> > >
> > > > In D65835#1623772 , @ABataev 
> > > > wrote:
> > > >
> > > > > In D65835#1623756 , @kkwli0 
> > > > > wrote:
> > > > >
> > > > > > In D65835#1622042 , 
> > > > > > @ABataev wrote:
> > > > > >
> > > > > > > > I want to be sure we're on the same page: For OpenMP 5.0, 
> > > > > > > > should we allow is_device_ptr with the private clauses?
> > > > > > >
> > > > > > > Yes, since it is allowed by the standard.
> > > > > >
> > > > > >
> > > > > > Umm ... I probably missed some earlier discussions!  What would be 
> > > > > > the behavior of the following code?
> > > > > >
> > > > > >   p = omp_target_alloc(...);
> > > > > >   #pragma omp target private(p) is_device_ptr(p)
> > > > > > p[...] = ...;   // crash or not?
> > > > > >
> > > > >
> > > > >
> > > > > It must crush, I assume. The main problem is that this construct is 
> > > > > allowed by the standard.
> > > >
> > > >
> > > > Yep. We should add a warning message for it.
> > >
> > >
> > > Upon further reflection, this is not clearly allowed by the standard. My 
> > > experience is that, when reading standards, sometimes things are 
> > > disallowed by contradiction (i.e., the standard does not define some 
> > > behavior, and what the standard does say that's relevant is self 
> > > contradictory). In this case, 2.19.3 says that list items which are 
> > > privatized (and which are used) undergo replacement (with new items 
> > > created as specified) while 2.12.5 says that "The is_device_ptr clause is 
> > > used to indicate that a list item is a device pointer already in the 
> > > device data environment and that it should be used directly." A given 
> > > list item cannot simultaneously be "used directly" (2.12.5) and also 
> > > undergo replacement: "Inside the construct, all references to the 
> > > original list item are replaced by references to a new list item received 
> > > by the task or SIMD lane" (2.19.3). Thus, it may be disallowed.
> >
> >
> > I think, this combination is still allowed. I assume that
> >
> >   #Pragma omp target parallel is_device_ptr(a) (a)
> >
> >
> > is the same as
> >
> >   #pragma omp target is_device_ptr(a)
> >   #pragma omp parallel (a)
> >
> >
> > i.e. datasharing clauses are applied to inner sub-regions, not the target 
> > region itself.
>
>
> With the parallel, that makes sense to me. In that case, however, I'd imagine 
> that the privitization works as normal and the code wouldn't crash. Right?


It leads to the use of uninitialized memory. We create private non-initialized 
copy of the pointer, not the copy of the data it addresses. It shall work with 
firstprivate clause.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D65835#1623903 , @ABataev wrote:

> In D65835#1623883 , @hfinkel wrote:
>
> > In D65835#1623814 , @hfinkel wrote:
> >
> > > In D65835#1623772 , @ABataev 
> > > wrote:
> > >
> > > > In D65835#1623756 , @kkwli0 
> > > > wrote:
> > > >
> > > > > In D65835#1622042 , @ABataev 
> > > > > wrote:
> > > > >
> > > > > > > I want to be sure we're on the same page: For OpenMP 5.0, should 
> > > > > > > we allow is_device_ptr with the private clauses?
> > > > > >
> > > > > > Yes, since it is allowed by the standard.
> > > > >
> > > > >
> > > > > Umm ... I probably missed some earlier discussions!  What would be 
> > > > > the behavior of the following code?
> > > > >
> > > > >   p = omp_target_alloc(...);
> > > > >   #pragma omp target private(p) is_device_ptr(p)
> > > > > p[...] = ...;   // crash or not?
> > > > >
> > > >
> > > >
> > > > It must crush, I assume. The main problem is that this construct is 
> > > > allowed by the standard.
> > >
> > >
> > > Yep. We should add a warning message for it.
> >
> >
> > Upon further reflection, this is not clearly allowed by the standard. My 
> > experience is that, when reading standards, sometimes things are disallowed 
> > by contradiction (i.e., the standard does not define some behavior, and 
> > what the standard does say that's relevant is self contradictory). In this 
> > case, 2.19.3 says that list items which are privatized (and which are used) 
> > undergo replacement (with new items created as specified) while 2.12.5 says 
> > that "The is_device_ptr clause is used to indicate that a list item is a 
> > device pointer already in the device data environment and that it should be 
> > used directly." A given list item cannot simultaneously be "used directly" 
> > (2.12.5) and also undergo replacement: "Inside the construct, all 
> > references to the original list item are replaced by references to a new 
> > list item received by the task or SIMD lane" (2.19.3). Thus, it may be 
> > disallowed.
>
>
> I think, this combination is still allowed. I assume that
>
>   #Pragma omp target parallel is_device_ptr(a) (a)
>
>
> is the same as
>
>   #pragma omp target is_device_ptr(a)
>   #pragma omp parallel (a)
>
>
> i.e. datasharing clauses are applied to inner sub-regions, not the target 
> region itself.


With the parallel, that makes sense to me. In that case, however, I'd imagine 
that the privitization works as normal and the code wouldn't crash. Right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65835#1623883 , @hfinkel wrote:

> In D65835#1623814 , @hfinkel wrote:
>
> > In D65835#1623772 , @ABataev wrote:
> >
> > > In D65835#1623756 , @kkwli0 
> > > wrote:
> > >
> > > > In D65835#1622042 , @ABataev 
> > > > wrote:
> > > >
> > > > > > I want to be sure we're on the same page: For OpenMP 5.0, should we 
> > > > > > allow is_device_ptr with the private clauses?
> > > > >
> > > > > Yes, since it is allowed by the standard.
> > > >
> > > >
> > > > Umm ... I probably missed some earlier discussions!  What would be the 
> > > > behavior of the following code?
> > > >
> > > >   p = omp_target_alloc(...);
> > > >   #pragma omp target private(p) is_device_ptr(p)
> > > > p[...] = ...;   // crash or not?
> > > >
> > >
> > >
> > > It must crush, I assume. The main problem is that this construct is 
> > > allowed by the standard.
> >
> >
> > Yep. We should add a warning message for it.
>
>
> Upon further reflection, this is not clearly allowed by the standard. My 
> experience is that, when reading standards, sometimes things are disallowed 
> by contradiction (i.e., the standard does not define some behavior, and what 
> the standard does say that's relevant is self contradictory). In this case, 
> 2.19.3 says that list items which are privatized (and which are used) undergo 
> replacement (with new items created as specified) while 2.12.5 says that "The 
> is_device_ptr clause is used to indicate that a list item is a device pointer 
> already in the device data environment and that it should be used directly." 
> A given list item cannot simultaneously be "used directly" (2.12.5) and also 
> undergo replacement: "Inside the construct, all references to the original 
> list item are replaced by references to a new list item received by the task 
> or SIMD lane" (2.19.3). Thus, it may be disallowed.


I think, this combination is still allowed. I assume that

  #Pragma omp target parallel is_device_ptr(a) (a)

is the same as

  #pragma omp target is_device_ptr(a)
  #pragma omp parallel (a)

i.e. datasharing clauses are applied to inner sub-regions, not the target 
region itself.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Kelvin Li via Phabricator via cfe-commits]

kkwli0 added a comment.

In D65835#1623883 , @hfinkel wrote:

> In D65835#1623814 , @hfinkel wrote:
>
> > In D65835#1623772 , @ABataev wrote:
> >
> > > In D65835#1623756 , @kkwli0 
> > > wrote:
> > >
> > > > In D65835#1622042 , @ABataev 
> > > > wrote:
> > > >
> > > > > > I want to be sure we're on the same page: For OpenMP 5.0, should we 
> > > > > > allow is_device_ptr with the private clauses?
> > > > >
> > > > > Yes, since it is allowed by the standard.
> > > >
> > > >
> > > > Umm ... I probably missed some earlier discussions!  What would be the 
> > > > behavior of the following code?
> > > >
> > > >   p = omp_target_alloc(...);
> > > >   #pragma omp target private(p) is_device_ptr(p)
> > > > p[...] = ...;   // crash or not?
> > > >
> > >
> > >
> > > It must crush, I assume. The main problem is that this construct is 
> > > allowed by the standard.
> >
> >
> > Yep. We should add a warning message for it.
>
>
> Upon further reflection, this is not clearly allowed by the standard. My 
> experience is that, when reading standards, sometimes things are disallowed 
> by contradiction (i.e., the standard does not define some behavior, and what 
> the standard does say that's relevant is self contradictory). In this case, 
> 2.19.3 says that list items which are privatized (and which are used) undergo 
> replacement (with new items created as specified) while 2.12.5 says that "The 
> is_device_ptr clause is used to indicate that a list item is a device pointer 
> already in the device data environment and that it should be used directly." 
> A given list item cannot simultaneously be "used directly" (2.12.5) and also 
> undergo replacement: "Inside the construct, all references to the original 
> list item are replaced by references to a new list item received by the task 
> or SIMD lane" (2.19.3). Thus, it may be disallowed.


That is what I thought.  Specifying these two clauses on the target construct 
creates ambiguity on which p it referred to inside the construct.  The private 
p or the pointer p that stores the device address?  I will work with the 
committee to fix the spec.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D65835#1623814 , @hfinkel wrote:

> In D65835#1623772 , @ABataev wrote:
>
> > In D65835#1623756 , @kkwli0 wrote:
> >
> > > In D65835#1622042 , @ABataev 
> > > wrote:
> > >
> > > > > I want to be sure we're on the same page: For OpenMP 5.0, should we 
> > > > > allow is_device_ptr with the private clauses?
> > > >
> > > > Yes, since it is allowed by the standard.
> > >
> > >
> > > Umm ... I probably missed some earlier discussions!  What would be the 
> > > behavior of the following code?
> > >
> > >   p = omp_target_alloc(...);
> > >   #pragma omp target private(p) is_device_ptr(p)
> > > p[...] = ...;   // crash or not?
> > >
> >
> >
> > It must crush, I assume. The main problem is that this construct is allowed 
> > by the standard.
>
>
> Yep. We should add a warning message for it.


Upon further reflection, this is not clearly allowed by the standard. My 
experience is that, when reading standards, sometimes things are disallowed by 
contradiction (i.e., the standard does not define some behavior, and what the 
standard does say that's relevant is self contradictory). In this case, 2.19.3 
says that list items which are privatized (and which are used) undergo 
replacement (with new items created as specified) while 2.12.5 says that "The 
is_device_ptr clause is used to indicate that a list item is a device pointer 
already in the device data environment and that it should be used directly." A 
given list item cannot simultaneously be "used directly" (2.12.5) and also 
undergo replacement: "Inside the construct, all references to the original list 
item are replaced by references to a new list item received by the task or SIMD 
lane" (2.19.3). Thus, it may be disallowed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D65835#1623858 , @ABataev wrote:

> In D65835#1623854 , @jdenny wrote:
>
> > In D65835#1623814 , @hfinkel wrote:
> >
> > > In D65835#1623772 , @ABataev 
> > > wrote:
> > >
> > > > In D65835#1623756 , @kkwli0 
> > > > wrote:
> > > >
> > > > > In D65835#1622042 , @ABataev 
> > > > > wrote:
> > > > >
> > > > > > > I want to be sure we're on the same page: For OpenMP 5.0, should 
> > > > > > > we allow is_device_ptr with the private clauses?
> > > > > >
> > > > > > Yes, since it is allowed by the standard.
> > > > >
> > > > >
> > > > > Umm ... I probably missed some earlier discussions!  What would be 
> > > > > the behavior of the following code?
> > > > >
> > > > >   p = omp_target_alloc(...);
> > > > >   #pragma omp target private(p) is_device_ptr(p)
> > > > > p[...] = ...;   // crash or not?
> > > > >
> > > >
> > > >
> > > > It must crush, I assume. The main problem is that this construct is 
> > > > allowed by the standard.
> > >
> > >
> > > Yep. We should add a warning message for it.
> >
> >
> > Clang currently doesn't permit is_device_ptr with firstprivate either, but 
> > I'm not aware of any reason to diagnose that.  Is there?  And then there 
> > are privatization clauses like linear and reduction
>
>
> I assume, we should allow all allowed datasharing clauses along with 
> is_device_ptr clause, including shared, linear, etc.


Agreed.  I was just wondering if anyone saw an obvious issue that might deserve 
a warning (as in the private plus is_device_ptr case).  I'm not planning to 
implement warnings right now.  I'm just curious for later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65835#1623854 , @jdenny wrote:

> In D65835#1623814 , @hfinkel wrote:
>
> > In D65835#1623772 , @ABataev wrote:
> >
> > > In D65835#1623756 , @kkwli0 
> > > wrote:
> > >
> > > > In D65835#1622042 , @ABataev 
> > > > wrote:
> > > >
> > > > > > I want to be sure we're on the same page: For OpenMP 5.0, should we 
> > > > > > allow is_device_ptr with the private clauses?
> > > > >
> > > > > Yes, since it is allowed by the standard.
> > > >
> > > >
> > > > Umm ... I probably missed some earlier discussions!  What would be the 
> > > > behavior of the following code?
> > > >
> > > >   p = omp_target_alloc(...);
> > > >   #pragma omp target private(p) is_device_ptr(p)
> > > > p[...] = ...;   // crash or not?
> > > >
> > >
> > >
> > > It must crush, I assume. The main problem is that this construct is 
> > > allowed by the standard.
> >
> >
> > Yep. We should add a warning message for it.
>
>
> Clang currently doesn't permit is_device_ptr with firstprivate either, but 
> I'm not aware of any reason to diagnose that.  Is there?  And then there are 
> privatization clauses like linear and reduction


I assume, we should allow all allowed datasharing clauses along with 
is_device_ptr clause, including shared, linear, etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D65835#1623814 , @hfinkel wrote:

> In D65835#1623772 , @ABataev wrote:
>
> > In D65835#1623756 , @kkwli0 wrote:
> >
> > > In D65835#1622042 , @ABataev 
> > > wrote:
> > >
> > > > > I want to be sure we're on the same page: For OpenMP 5.0, should we 
> > > > > allow is_device_ptr with the private clauses?
> > > >
> > > > Yes, since it is allowed by the standard.
> > >
> > >
> > > Umm ... I probably missed some earlier discussions!  What would be the 
> > > behavior of the following code?
> > >
> > >   p = omp_target_alloc(...);
> > >   #pragma omp target private(p) is_device_ptr(p)
> > > p[...] = ...;   // crash or not?
> > >
> >
> >
> > It must crush, I assume. The main problem is that this construct is allowed 
> > by the standard.
>
>
> Yep. We should add a warning message for it.


Clang currently doesn't permit is_device_ptr with firstprivate either, but I'm 
not aware of any reason to diagnose that.  Is there?  And then there are 
privatization clauses like linear and reduction


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D65835#1623772 , @ABataev wrote:

> In D65835#1623756 , @kkwli0 wrote:
>
> > In D65835#1622042 , @ABataev wrote:
> >
> > > > I want to be sure we're on the same page: For OpenMP 5.0, should we 
> > > > allow is_device_ptr with the private clauses?
> > >
> > > Yes, since it is allowed by the standard.
> >
> >
> > Umm ... I probably missed some earlier discussions!  What would be the 
> > behavior of the following code?
> >
> >   p = omp_target_alloc(...);
> >   #pragma omp target private(p) is_device_ptr(p)
> > p[...] = ...;   // crash or not?
> >
>
>
> It must crush, I assume. The main problem is that this construct is allowed 
> by the standard.


Yep. We should add a warning message for it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65835#1623756 , @kkwli0 wrote:

> In D65835#1622042 , @ABataev wrote:
>
> > > I want to be sure we're on the same page: For OpenMP 5.0, should we allow 
> > > is_device_ptr with the private clauses?
> >
> > Yes, since it is allowed by the standard.
>
>
> Umm ... I probably missed some earlier discussions!  What would be the 
> behavior of the following code?
>
>   p = omp_target_alloc(...);
>   #pragma omp target private(p) is_device_ptr(p)
> p[...] = ...;   // crash or not?
>


It must crush, I assume. The main problem is that this construct is allowed by 
the standard.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Kelvin Li via Phabricator via cfe-commits]

kkwli0 added a comment.

In D65835#1622042 , @ABataev wrote:

> > I want to be sure we're on the same page: For OpenMP 5.0, should we allow 
> > is_device_ptr with the private clauses?
>
> Yes, since it is allowed by the standard.


Umm ... I probably missed some earlier discussions!  What would be the behavior 
of the following code?

  p = omp_target_alloc(...);
  #pragma omp target private(p) is_device_ptr(p)
p[...] = ...;   // crash or not?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

> I want to be sure we're on the same page: For OpenMP 5.0, should we allow 
> is_device_ptr with the private clauses?

Yes, since it is allowed by the standard.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D65835#1621202 , @ABataev wrote:

> >> See 2.19.7 Data-Mapping Attribute Rules, Clauses, and Directives
> > 
> > I looked again.  I'm still not finding any text in that section that 
> > implies is_device_ptr follows the same restrictions as map.  Can you please 
> > cite specific lines of text instead of an entire section?  Thanks for your 
> > help.
>
> Ah, it is only in OpenMP 5.0 anв restrictions for the map clause are for map 
> clause only. Then we should allow `is_device_ptr` with the private clauses 
> for OpenMP 4.5.


I want to be sure we're on the same page: For OpenMP 5.0, should we allow 
`is_device_ptr` with the private clauses?

> Better to do this in a separate patch only for `is_device_ptr`.

I'll remove it from this one.  Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

jdenny wrote:
> hfinkel wrote:
> > ABataev wrote:
> > > hfinkel wrote:
> > > > ABataev wrote:
> > > > > hfinkel wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > > I think this should cause an error or at least a 
> > > > > > > > > > > > > > warning. Telling the compiler `ps` is a device 
> > > > > > > > > > > > > > pointer only to create a local uninitialized 
> > > > > > > > > > > > > > shadowing variable seems like an error to me.
> > > > > > > > > > > > > It is allowed according to OpenMP 5.0. Private copy 
> > > > > > > > > > > > > must be created in the context of the parallel 
> > > > > > > > > > > > > region, not the target region. So, for OpenMP 5.0 we 
> > > > > > > > > > > > > should not emit error here.
> > > > > > > > > > > > What does that mean and how does that affect my 
> > > > > > > > > > > > reasoning?
> > > > > > > > > > > It means, that for OpenMP 5.0 we should emit a 
> > > > > > > > > > > warning/error here. It is allowed according to the 
> > > > > > > > > > > standard, we should allow it too.
> > > > > > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > > > > > 
> > > > > > > > > > The last answer contradicts what you said earlier. I expect 
> > > > > > > > > > there is a *not* missing, correct?
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Assuming you do not want an error, which is fine, I still 
> > > > > > > > > > think a warning is appropriate as it seems to me there is 
> > > > > > > > > > never a reason to have a `is_device_ptr` clause for a 
> > > > > > > > > > private value. I mean, it is either a bug by the 
> > > > > > > > > > programmer, e.g., 5 letters of `firstprivate` went missing, 
> > > > > > > > > > or simply nonsensical code for which we warn in other 
> > > > > > > > > > situations as well.
> > > > > > > > > Missed `not`.
> > > > > > > > > These kind of construct are explicitly allowed in OpenMP. And 
> > > > > > > > > we should obey the standard unconditionally.
> > > > > > > > > Plus, there might be situations where user may require it 
> > > > > > > > > explicitly. For example, the device pointer is dereferenced 
> > > > > > > > > in one of the clauses for the subregions but in the deeper 
> > > > > > > > > subregion it might be used as a private pointer. Why we 
> > > > > > > > > should emit a warning here?
> > > > > > > > If you have a different situation, e.g., the one you describe, 
> > > > > > > > you should not have a warning. Though, that is not the point. 
> > > > > > > > If you have the situation above (single directive), as per my 
> > > > > > > > reasoning, there doesn't seem to be a sensible use case. If you 
> > > > > > > > have one and we should create an explicit test for it.
> > > > > > > > 
> > > > > > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > > > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > > > > > And we should obey the standard unconditionally.
> > > > > > > > Nobody says we should not. We warn people all the time even if 
> > > > > > > > it is valid code.
> > > > > > > Warnings may prevent successful compilation in some cases, e.g. 
> > > > > > > when warnings are treated as errors. Why we should emit a warning 
> > > > > > > if the construct is allowed by the standard? Ask to change the 
> > > > > > > standard if you did not agree with it.
> > > > > > Warnings are specifically for constructs which are legal, but 
> > > > > > likely wrong (i.e., will not do what the user likely intended). 
> > > > > > Treating warnings as errors is not a conforming compilation mode - 
> > > > > > by design (specifically because that will reject conforming 
> > > > > > programs). Thus...
> > > > > > 
> > > > > > > Why we should emit a warning if the construct is allowed by the 
> > > > > > > standard? Ask to change the standard if you did not agree with it.
> > > > > > 
> > > > > > This is the wrong way to approach this. Warnings are specifically 
> > > > > > for legal code. They help users prevent errors, however, in cases 
> > > > > > where that legal code is likely problematic or won't do what the 
> > > > > > user intends.
> > > > > > 
> > > > > Ok, we could emit 

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65835#1621168 , @jdenny wrote:

> In D65835#1619560 , @ABataev wrote:
>
> > In D65835#1619549 , @jdenny wrote:
> >
> > > In D65835#1619526 , @ABataev 
> > > wrote:
> > >
> > > > > Maybe, but I haven't found any statement in either version that 
> > > > > states that map restrictions apply to is_device_ptr.
> > > >
> > > > `is_device_ptr` is a kind of mapping clause. I assume we can extend the 
> > > > restrictions for `map` clause to this clause too.
> > >
> > >
> > > I'd like to understand this better.  Is there something from the spec we 
> > > can quote in the code?
> >
> >
> > See 2.19.7 Data-Mapping Attribute Rules, Clauses, and Directives
>
>
> I looked again.  I'm still not finding any text in that section that implies 
> is_device_ptr follows the same restrictions as map.  Can you please cite 
> specific lines of text instead of an entire section?  Thanks for your help.


Ah, it is only in OpenMP 5.0 anв restrictions for the map clause are for map 
clause only. Then we should allow `is_device_ptr` with the private clauses for 
OpenMP 4.5.
Better to do this in a separate patch only for `is_device_ptr`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked 2 inline comments as done.
jdenny added a comment.

In D65835#1619560 , @ABataev wrote:

> In D65835#1619549 , @jdenny wrote:
>
> > In D65835#1619526 , @ABataev wrote:
> >
> > > > Maybe, but I haven't found any statement in either version that states 
> > > > that map restrictions apply to is_device_ptr.
> > >
> > > `is_device_ptr` is a kind of mapping clause. I assume we can extend the 
> > > restrictions for `map` clause to this clause too.
> >
> >
> > I'd like to understand this better.  Is there something from the spec we 
> > can quote in the code?
>
>
> See 2.19.7 Data-Mapping Attribute Rules, Clauses, and Directives


I looked again.  I'm still not finding any text in that section that implies 
is_device_ptr follows the same restrictions as map.  Can you please cite 
specific lines of text instead of an entire section?  Thanks for your help.




Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

hfinkel wrote:
> ABataev wrote:
> > hfinkel wrote:
> > > ABataev wrote:
> > > > hfinkel wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > > I think this should cause an error or at least a 
> > > > > > > > > > > > > warning. Telling the compiler `ps` is a device 
> > > > > > > > > > > > > pointer only to create a local uninitialized 
> > > > > > > > > > > > > shadowing variable seems like an error to me.
> > > > > > > > > > > > It is allowed according to OpenMP 5.0. Private copy 
> > > > > > > > > > > > must be created in the context of the parallel region, 
> > > > > > > > > > > > not the target region. So, for OpenMP 5.0 we should not 
> > > > > > > > > > > > emit error here.
> > > > > > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > > > > > It means, that for OpenMP 5.0 we should emit a 
> > > > > > > > > > warning/error here. It is allowed according to the 
> > > > > > > > > > standard, we should allow it too.
> > > > > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > > > > 
> > > > > > > > > The last answer contradicts what you said earlier. I expect 
> > > > > > > > > there is a *not* missing, correct?
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Assuming you do not want an error, which is fine, I still 
> > > > > > > > > think a warning is appropriate as it seems to me there is 
> > > > > > > > > never a reason to have a `is_device_ptr` clause for a private 
> > > > > > > > > value. I mean, it is either a bug by the programmer, e.g., 5 
> > > > > > > > > letters of `firstprivate` went missing, or simply nonsensical 
> > > > > > > > > code for which we warn in other situations as well.
> > > > > > > > Missed `not`.
> > > > > > > > These kind of construct are explicitly allowed in OpenMP. And 
> > > > > > > > we should obey the standard unconditionally.
> > > > > > > > Plus, there might be situations where user may require it 
> > > > > > > > explicitly. For example, the device pointer is dereferenced in 
> > > > > > > > one of the clauses for the subregions but in the deeper 
> > > > > > > > subregion it might be used as a private pointer. Why we should 
> > > > > > > > emit a warning here?
> > > > > > > If you have a different situation, e.g., the one you describe, 
> > > > > > > you should not have a warning. Though, that is not the point. If 
> > > > > > > you have the situation above (single directive), as per my 
> > > > > > > reasoning, there doesn't seem to be a sensible use case. If you 
> > > > > > > have one and we should create an explicit test for it.
> > > > > > > 
> > > > > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > > > > And we should obey the standard unconditionally.
> > > > > > > Nobody says we should not. We warn people all the time even if it 
> > > > > > > is valid code.
> > > > > > Warnings may prevent successful compilation in some cases, e.g. 
> > > > > > when warnings are treated as errors. Why we should emit a warning 
> > > > > > if the construct is allowed by the standard? Ask to change the 
> > > > > > standard if you did not agree with it.
> > > > > Warnings are specifically for 

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

ABataev wrote:
> hfinkel wrote:
> > ABataev wrote:
> > > hfinkel wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > I think this should cause an error or at least a 
> > > > > > > > > > > > warning. Telling the compiler `ps` is a device pointer 
> > > > > > > > > > > > only to create a local uninitialized shadowing variable 
> > > > > > > > > > > > seems like an error to me.
> > > > > > > > > > > It is allowed according to OpenMP 5.0. Private copy must 
> > > > > > > > > > > be created in the context of the parallel region, not the 
> > > > > > > > > > > target region. So, for OpenMP 5.0 we should not emit 
> > > > > > > > > > > error here.
> > > > > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > > > > It means, that for OpenMP 5.0 we should emit a warning/error 
> > > > > > > > > here. It is allowed according to the standard, we should 
> > > > > > > > > allow it too.
> > > > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > > > 
> > > > > > > > The last answer contradicts what you said earlier. I expect 
> > > > > > > > there is a *not* missing, correct?
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Assuming you do not want an error, which is fine, I still think 
> > > > > > > > a warning is appropriate as it seems to me there is never a 
> > > > > > > > reason to have a `is_device_ptr` clause for a private value. I 
> > > > > > > > mean, it is either a bug by the programmer, e.g., 5 letters of 
> > > > > > > > `firstprivate` went missing, or simply nonsensical code for 
> > > > > > > > which we warn in other situations as well.
> > > > > > > Missed `not`.
> > > > > > > These kind of construct are explicitly allowed in OpenMP. And we 
> > > > > > > should obey the standard unconditionally.
> > > > > > > Plus, there might be situations where user may require it 
> > > > > > > explicitly. For example, the device pointer is dereferenced in 
> > > > > > > one of the clauses for the subregions but in the deeper subregion 
> > > > > > > it might be used as a private pointer. Why we should emit a 
> > > > > > > warning here?
> > > > > > If you have a different situation, e.g., the one you describe, you 
> > > > > > should not have a warning. Though, that is not the point. If you 
> > > > > > have the situation above (single directive), as per my reasoning, 
> > > > > > there doesn't seem to be a sensible use case. If you have one and 
> > > > > > we should create an explicit test for it.
> > > > > > 
> > > > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > > > And we should obey the standard unconditionally.
> > > > > > Nobody says we should not. We warn people all the time even if it 
> > > > > > is valid code.
> > > > > Warnings may prevent successful compilation in some cases, e.g. when 
> > > > > warnings are treated as errors. Why we should emit a warning if the 
> > > > > construct is allowed by the standard? Ask to change the standard if 
> > > > > you did not agree with it.
> > > > Warnings are specifically for constructs which are legal, but likely 
> > > > wrong (i.e., will not do what the user likely intended). Treating 
> > > > warnings as errors is not a conforming compilation mode - by design 
> > > > (specifically because that will reject conforming programs). Thus...
> > > > 
> > > > > Why we should emit a warning if the construct is allowed by the 
> > > > > standard? Ask to change the standard if you did not agree with it.
> > > > 
> > > > This is the wrong way to approach this. Warnings are specifically for 
> > > > legal code. They help users prevent errors, however, in cases where 
> > > > that legal code is likely problematic or won't do what the user intends.
> > > > 
> > > Ok, we could emit wqrnings in some cases. But better to do it in the 
> > > separate patches. Each particular case requires additional analysis.
> > > 
> > > > This is the wrong way to approach this.
> > > 
> > > I don't think so. If some cases are really meaningless, better to ask to 
> > > prohibit them in the standard. It is always a good idea to change the 
> > > requirements first, if you think that some 

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

hfinkel wrote:
> ABataev wrote:
> > hfinkel wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > I think this should cause an error or at least a warning. 
> > > > > > > > > > > Telling the compiler `ps` is a device pointer only to 
> > > > > > > > > > > create a local uninitialized shadowing variable seems 
> > > > > > > > > > > like an error to me.
> > > > > > > > > > It is allowed according to OpenMP 5.0. Private copy must be 
> > > > > > > > > > created in the context of the parallel region, not the 
> > > > > > > > > > target region. So, for OpenMP 5.0 we should not emit error 
> > > > > > > > > > here.
> > > > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > > > It means, that for OpenMP 5.0 we should emit a warning/error 
> > > > > > > > here. It is allowed according to the standard, we should allow 
> > > > > > > > it too.
> > > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > > 
> > > > > > > The last answer contradicts what you said earlier. I expect there 
> > > > > > > is a *not* missing, correct?
> > > > > > > 
> > > > > > > 
> > > > > > > Assuming you do not want an error, which is fine, I still think a 
> > > > > > > warning is appropriate as it seems to me there is never a reason 
> > > > > > > to have a `is_device_ptr` clause for a private value. I mean, it 
> > > > > > > is either a bug by the programmer, e.g., 5 letters of 
> > > > > > > `firstprivate` went missing, or simply nonsensical code for which 
> > > > > > > we warn in other situations as well.
> > > > > > Missed `not`.
> > > > > > These kind of construct are explicitly allowed in OpenMP. And we 
> > > > > > should obey the standard unconditionally.
> > > > > > Plus, there might be situations where user may require it 
> > > > > > explicitly. For example, the device pointer is dereferenced in one 
> > > > > > of the clauses for the subregions but in the deeper subregion it 
> > > > > > might be used as a private pointer. Why we should emit a warning 
> > > > > > here?
> > > > > If you have a different situation, e.g., the one you describe, you 
> > > > > should not have a warning. Though, that is not the point. If you have 
> > > > > the situation above (single directive), as per my reasoning, there 
> > > > > doesn't seem to be a sensible use case. If you have one and we should 
> > > > > create an explicit test for it.
> > > > > 
> > > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > > And we should obey the standard unconditionally.
> > > > > Nobody says we should not. We warn people all the time even if it is 
> > > > > valid code.
> > > > Warnings may prevent successful compilation in some cases, e.g. when 
> > > > warnings are treated as errors. Why we should emit a warning if the 
> > > > construct is allowed by the standard? Ask to change the standard if you 
> > > > did not agree with it.
> > > Warnings are specifically for constructs which are legal, but likely 
> > > wrong (i.e., will not do what the user likely intended). Treating 
> > > warnings as errors is not a conforming compilation mode - by design 
> > > (specifically because that will reject conforming programs). Thus...
> > > 
> > > > Why we should emit a warning if the construct is allowed by the 
> > > > standard? Ask to change the standard if you did not agree with it.
> > > 
> > > This is the wrong way to approach this. Warnings are specifically for 
> > > legal code. They help users prevent errors, however, in cases where that 
> > > legal code is likely problematic or won't do what the user intends.
> > > 
> > Ok, we could emit wqrnings in some cases. But better to do it in the 
> > separate patches. Each particular case requires additional analysis.
> > 
> > > This is the wrong way to approach this.
> > 
> > I don't think so. If some cases are really meaningless, better to ask to 
> > prohibit them in the standard. It is always a good idea to change the 
> > requirements first, if you think that some scenarios are not described 
> > correctly rather than do the changes in the code. It leads to different 
> > behavior of different compilers in the same situation and it is not 

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

ABataev wrote:
> hfinkel wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > I think this should cause an error or at least a warning. 
> > > > > > > > > > Telling the compiler `ps` is a device pointer only to 
> > > > > > > > > > create a local uninitialized shadowing variable seems like 
> > > > > > > > > > an error to me.
> > > > > > > > > It is allowed according to OpenMP 5.0. Private copy must be 
> > > > > > > > > created in the context of the parallel region, not the target 
> > > > > > > > > region. So, for OpenMP 5.0 we should not emit error here.
> > > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > > It means, that for OpenMP 5.0 we should emit a warning/error 
> > > > > > > here. It is allowed according to the standard, we should allow it 
> > > > > > > too.
> > > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > > 
> > > > > > The last answer contradicts what you said earlier. I expect there 
> > > > > > is a *not* missing, correct?
> > > > > > 
> > > > > > 
> > > > > > Assuming you do not want an error, which is fine, I still think a 
> > > > > > warning is appropriate as it seems to me there is never a reason to 
> > > > > > have a `is_device_ptr` clause for a private value. I mean, it is 
> > > > > > either a bug by the programmer, e.g., 5 letters of `firstprivate` 
> > > > > > went missing, or simply nonsensical code for which we warn in other 
> > > > > > situations as well.
> > > > > Missed `not`.
> > > > > These kind of construct are explicitly allowed in OpenMP. And we 
> > > > > should obey the standard unconditionally.
> > > > > Plus, there might be situations where user may require it explicitly. 
> > > > > For example, the device pointer is dereferenced in one of the clauses 
> > > > > for the subregions but in the deeper subregion it might be used as a 
> > > > > private pointer. Why we should emit a warning here?
> > > > If you have a different situation, e.g., the one you describe, you 
> > > > should not have a warning. Though, that is not the point. If you have 
> > > > the situation above (single directive), as per my reasoning, there 
> > > > doesn't seem to be a sensible use case. If you have one and we should 
> > > > create an explicit test for it.
> > > > 
> > > > > These kind of construct are explicitly allowed in OpenMP.
> > > > `explicitly allowed` != `not forbidded` (yet)
> > > > > And we should obey the standard unconditionally.
> > > > Nobody says we should not. We warn people all the time even if it is 
> > > > valid code.
> > > Warnings may prevent successful compilation in some cases, e.g. when 
> > > warnings are treated as errors. Why we should emit a warning if the 
> > > construct is allowed by the standard? Ask to change the standard if you 
> > > did not agree with it.
> > Warnings are specifically for constructs which are legal, but likely wrong 
> > (i.e., will not do what the user likely intended). Treating warnings as 
> > errors is not a conforming compilation mode - by design (specifically 
> > because that will reject conforming programs). Thus...
> > 
> > > Why we should emit a warning if the construct is allowed by the standard? 
> > > Ask to change the standard if you did not agree with it.
> > 
> > This is the wrong way to approach this. Warnings are specifically for legal 
> > code. They help users prevent errors, however, in cases where that legal 
> > code is likely problematic or won't do what the user intends.
> > 
> Ok, we could emit wqrnings in some cases. But better to do it in the separate 
> patches. Each particular case requires additional analysis.
> 
> > This is the wrong way to approach this.
> 
> I don't think so. If some cases are really meaningless, better to ask to 
> prohibit them in the standard. It is always a good idea to change the 
> requirements first, if you think that some scenarios are not described 
> correctly rather than do the changes in the code. It leads to different 
> behavior of different compilers in the same situation and it is not good for 
> the users.
> I don't think so. If some cases are really meaningless, better to ask to 
> prohibit them in the standard. It is always a good idea to change the 
> requirements first, if you 

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

hfinkel wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > jdoerfert wrote:
> > > > > > > > > I think this should cause an error or at least a warning. 
> > > > > > > > > Telling the compiler `ps` is a device pointer only to create 
> > > > > > > > > a local uninitialized shadowing variable seems like an error 
> > > > > > > > > to me.
> > > > > > > > It is allowed according to OpenMP 5.0. Private copy must be 
> > > > > > > > created in the context of the parallel region, not the target 
> > > > > > > > region. So, for OpenMP 5.0 we should not emit error here.
> > > > > > > What does that mean and how does that affect my reasoning?
> > > > > > It means, that for OpenMP 5.0 we should emit a warning/error here. 
> > > > > > It is allowed according to the standard, we should allow it too.
> > > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > > 
> > > > > The last answer contradicts what you said earlier. I expect there is 
> > > > > a *not* missing, correct?
> > > > > 
> > > > > 
> > > > > Assuming you do not want an error, which is fine, I still think a 
> > > > > warning is appropriate as it seems to me there is never a reason to 
> > > > > have a `is_device_ptr` clause for a private value. I mean, it is 
> > > > > either a bug by the programmer, e.g., 5 letters of `firstprivate` 
> > > > > went missing, or simply nonsensical code for which we warn in other 
> > > > > situations as well.
> > > > Missed `not`.
> > > > These kind of construct are explicitly allowed in OpenMP. And we should 
> > > > obey the standard unconditionally.
> > > > Plus, there might be situations where user may require it explicitly. 
> > > > For example, the device pointer is dereferenced in one of the clauses 
> > > > for the subregions but in the deeper subregion it might be used as a 
> > > > private pointer. Why we should emit a warning here?
> > > If you have a different situation, e.g., the one you describe, you should 
> > > not have a warning. Though, that is not the point. If you have the 
> > > situation above (single directive), as per my reasoning, there doesn't 
> > > seem to be a sensible use case. If you have one and we should create an 
> > > explicit test for it.
> > > 
> > > > These kind of construct are explicitly allowed in OpenMP.
> > > `explicitly allowed` != `not forbidded` (yet)
> > > > And we should obey the standard unconditionally.
> > > Nobody says we should not. We warn people all the time even if it is 
> > > valid code.
> > Warnings may prevent successful compilation in some cases, e.g. when 
> > warnings are treated as errors. Why we should emit a warning if the 
> > construct is allowed by the standard? Ask to change the standard if you did 
> > not agree with it.
> Warnings are specifically for constructs which are legal, but likely wrong 
> (i.e., will not do what the user likely intended). Treating warnings as 
> errors is not a conforming compilation mode - by design (specifically because 
> that will reject conforming programs). Thus...
> 
> > Why we should emit a warning if the construct is allowed by the standard? 
> > Ask to change the standard if you did not agree with it.
> 
> This is the wrong way to approach this. Warnings are specifically for legal 
> code. They help users prevent errors, however, in cases where that legal code 
> is likely problematic or won't do what the user intends.
> 
Ok, we could emit wqrnings in some cases. But better to do it in the separate 
patches. Each particular case requires additional analysis.

> This is the wrong way to approach this.

I don't think so. If some cases are really meaningless, better to ask to 
prohibit them in the standard. It is always a good idea to change the 
requirements first, if you think that some scenarios are not described 
correctly rather than do the changes in the code. It leads to different 
behavior of different compilers in the same situation and it is not good for 
the users.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > I think this should cause an error or at least a warning. 
> > > > > > > > Telling the compiler `ps` is a device pointer only to create a 
> > > > > > > > local uninitialized shadowing variable seems like an error to 
> > > > > > > > me.
> > > > > > > It is allowed according to OpenMP 5.0. Private copy must be 
> > > > > > > created in the context of the parallel region, not the target 
> > > > > > > region. So, for OpenMP 5.0 we should not emit error here.
> > > > > > What does that mean and how does that affect my reasoning?
> > > > > It means, that for OpenMP 5.0 we should emit a warning/error here. It 
> > > > > is allowed according to the standard, we should allow it too.
> > > > > So, for OpenMP 5.0 we should not emit error here.
> > > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > > 
> > > > The last answer contradicts what you said earlier. I expect there is a 
> > > > *not* missing, correct?
> > > > 
> > > > 
> > > > Assuming you do not want an error, which is fine, I still think a 
> > > > warning is appropriate as it seems to me there is never a reason to 
> > > > have a `is_device_ptr` clause for a private value. I mean, it is either 
> > > > a bug by the programmer, e.g., 5 letters of `firstprivate` went 
> > > > missing, or simply nonsensical code for which we warn in other 
> > > > situations as well.
> > > Missed `not`.
> > > These kind of construct are explicitly allowed in OpenMP. And we should 
> > > obey the standard unconditionally.
> > > Plus, there might be situations where user may require it explicitly. For 
> > > example, the device pointer is dereferenced in one of the clauses for the 
> > > subregions but in the deeper subregion it might be used as a private 
> > > pointer. Why we should emit a warning here?
> > If you have a different situation, e.g., the one you describe, you should 
> > not have a warning. Though, that is not the point. If you have the 
> > situation above (single directive), as per my reasoning, there doesn't seem 
> > to be a sensible use case. If you have one and we should create an explicit 
> > test for it.
> > 
> > > These kind of construct are explicitly allowed in OpenMP.
> > `explicitly allowed` != `not forbidded` (yet)
> > > And we should obey the standard unconditionally.
> > Nobody says we should not. We warn people all the time even if it is valid 
> > code.
> Warnings may prevent successful compilation in some cases, e.g. when warnings 
> are treated as errors. Why we should emit a warning if the construct is 
> allowed by the standard? Ask to change the standard if you did not agree with 
> it.
Warnings are specifically for constructs which are legal, but likely wrong 
(i.e., will not do what the user likely intended). Treating warnings as errors 
is not a conforming compilation mode - by design (specifically because that 
will reject conforming programs). Thus...

> Why we should emit a warning if the construct is allowed by the standard? Ask 
> to change the standard if you did not agree with it.

This is the wrong way to approach this. Warnings are specifically for legal 
code. They help users prevent errors, however, in cases where that legal code 
is likely problematic or won't do what the user intends.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > I think this should cause an error or at least a warning. Telling 
> > > > > > > the compiler `ps` is a device pointer only to create a local 
> > > > > > > uninitialized shadowing variable seems like an error to me.
> > > > > > It is allowed according to OpenMP 5.0. Private copy must be created 
> > > > > > in the context of the parallel region, not the target region. So, 
> > > > > > for OpenMP 5.0 we should not emit error here.
> > > > > What does that mean and how does that affect my reasoning?
> > > > It means, that for OpenMP 5.0 we should emit a warning/error here. It 
> > > > is allowed according to the standard, we should allow it too.
> > > > So, for OpenMP 5.0 we should not emit error here.
> > > > that for OpenMP 5.0 we should emit a warning/error here.
> > > 
> > > The last answer contradicts what you said earlier. I expect there is a 
> > > *not* missing, correct?
> > > 
> > > 
> > > Assuming you do not want an error, which is fine, I still think a warning 
> > > is appropriate as it seems to me there is never a reason to have a 
> > > `is_device_ptr` clause for a private value. I mean, it is either a bug by 
> > > the programmer, e.g., 5 letters of `firstprivate` went missing, or simply 
> > > nonsensical code for which we warn in other situations as well.
> > Missed `not`.
> > These kind of construct are explicitly allowed in OpenMP. And we should 
> > obey the standard unconditionally.
> > Plus, there might be situations where user may require it explicitly. For 
> > example, the device pointer is dereferenced in one of the clauses for the 
> > subregions but in the deeper subregion it might be used as a private 
> > pointer. Why we should emit a warning here?
> If you have a different situation, e.g., the one you describe, you should not 
> have a warning. Though, that is not the point. If you have the situation 
> above (single directive), as per my reasoning, there doesn't seem to be a 
> sensible use case. If you have one and we should create an explicit test for 
> it.
> 
> > These kind of construct are explicitly allowed in OpenMP.
> `explicitly allowed` != `not forbidded` (yet)
> > And we should obey the standard unconditionally.
> Nobody says we should not. We warn people all the time even if it is valid 
> code.
Warnings may prevent successful compilation in some cases, e.g. when warnings 
are treated as errors. Why we should emit a warning if the construct is allowed 
by the standard? Ask to change the standard if you did not agree with it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > I think this should cause an error or at least a warning. Telling 
> > > > > > the compiler `ps` is a device pointer only to create a local 
> > > > > > uninitialized shadowing variable seems like an error to me.
> > > > > It is allowed according to OpenMP 5.0. Private copy must be created 
> > > > > in the context of the parallel region, not the target region. So, for 
> > > > > OpenMP 5.0 we should not emit error here.
> > > > What does that mean and how does that affect my reasoning?
> > > It means, that for OpenMP 5.0 we should emit a warning/error here. It is 
> > > allowed according to the standard, we should allow it too.
> > > So, for OpenMP 5.0 we should not emit error here.
> > > that for OpenMP 5.0 we should emit a warning/error here.
> > 
> > The last answer contradicts what you said earlier. I expect there is a 
> > *not* missing, correct?
> > 
> > 
> > Assuming you do not want an error, which is fine, I still think a warning 
> > is appropriate as it seems to me there is never a reason to have a 
> > `is_device_ptr` clause for a private value. I mean, it is either a bug by 
> > the programmer, e.g., 5 letters of `firstprivate` went missing, or simply 
> > nonsensical code for which we warn in other situations as well.
> Missed `not`.
> These kind of construct are explicitly allowed in OpenMP. And we should obey 
> the standard unconditionally.
> Plus, there might be situations where user may require it explicitly. For 
> example, the device pointer is dereferenced in one of the clauses for the 
> subregions but in the deeper subregion it might be used as a private pointer. 
> Why we should emit a warning here?
If you have a different situation, e.g., the one you describe, you should not 
have a warning. Though, that is not the point. If you have the situation above 
(single directive), as per my reasoning, there doesn't seem to be a sensible 
use case. If you have one and we should create an explicit test for it.

> These kind of construct are explicitly allowed in OpenMP.
`explicitly allowed` != `not forbidded` (yet)
> And we should obey the standard unconditionally.
Nobody says we should not. We warn people all the time even if it is valid code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > I think this should cause an error or at least a warning. Telling the 
> > > > > compiler `ps` is a device pointer only to create a local 
> > > > > uninitialized shadowing variable seems like an error to me.
> > > > It is allowed according to OpenMP 5.0. Private copy must be created in 
> > > > the context of the parallel region, not the target region. So, for 
> > > > OpenMP 5.0 we should not emit error here.
> > > What does that mean and how does that affect my reasoning?
> > It means, that for OpenMP 5.0 we should emit a warning/error here. It is 
> > allowed according to the standard, we should allow it too.
> > So, for OpenMP 5.0 we should not emit error here.
> > that for OpenMP 5.0 we should emit a warning/error here.
> 
> The last answer contradicts what you said earlier. I expect there is a *not* 
> missing, correct?
> 
> 
> Assuming you do not want an error, which is fine, I still think a warning is 
> appropriate as it seems to me there is never a reason to have a 
> `is_device_ptr` clause for a private value. I mean, it is either a bug by the 
> programmer, e.g., 5 letters of `firstprivate` went missing, or simply 
> nonsensical code for which we warn in other situations as well.
Missed `not`.
These kind of construct are explicitly allowed in OpenMP. And we should obey 
the standard unconditionally.
Plus, there might be situations where user may require it explicitly. For 
example, the device pointer is dereferenced in one of the clauses for the 
subregions but in the deeper subregion it might be used as a private pointer. 
Why we should emit a warning here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > I think this should cause an error or at least a warning. Telling the 
> > > > compiler `ps` is a device pointer only to create a local uninitialized 
> > > > shadowing variable seems like an error to me.
> > > It is allowed according to OpenMP 5.0. Private copy must be created in 
> > > the context of the parallel region, not the target region. So, for OpenMP 
> > > 5.0 we should not emit error here.
> > What does that mean and how does that affect my reasoning?
> It means, that for OpenMP 5.0 we should emit a warning/error here. It is 
> allowed according to the standard, we should allow it too.
> So, for OpenMP 5.0 we should not emit error here.
> that for OpenMP 5.0 we should emit a warning/error here.

The last answer contradicts what you said earlier. I expect there is a *not* 
missing, correct?


Assuming you do not want an error, which is fine, I still think a warning is 
appropriate as it seems to me there is never a reason to have a `is_device_ptr` 
clause for a private value. I mean, it is either a bug by the programmer, e.g., 
5 letters of `firstprivate` went missing, or simply nonsensical code for which 
we warn in other situations as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D65835#1619549 , @jdenny wrote:

> In D65835#1619526 , @ABataev wrote:
>
> > > Maybe, but I haven't found any statement in either version that states 
> > > that map restrictions apply to is_device_ptr.
> >
> > `is_device_ptr` is a kind of mapping clause. I assume we can extend the 
> > restrictions for `map` clause to this clause too.
>
>
> I'd like to understand this better.  Is there something from the spec we can 
> quote in the code?


See 2.19.7 Data-Mapping Attribute Rules, Clauses, and Directives

> 
> 
>>> Another question is whether the restriction would make sense. For example, 
>>> does it ever make sense to specify both is_device_ptr and firstprivate for 
>>> the same variable on a target construct?
>> 
>> On a `target` construct - no.
> 
> Why not?

It is meaningless. That's why it is prohibited in OpenMP 5.0 and allowed only 
for the combined constructs. These private clauses are applied to inner 
subconstructs.
For example, `target parallel map(p) private(p)`. In the context of `target` 
region the variable is mapped while in the `parallel` context it is private.
For `target map(p) private(p)` it is absolutely meaningless.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D65835#1619526 , @ABataev wrote:

> > Maybe, but I haven't found any statement in either version that states that 
> > map restrictions apply to is_device_ptr.
>
> `is_device_ptr` is a kind of mapping clause. I assume we can extend the 
> restrictions for `map` clause to this clause too.


I'd like to understand this better.  Is there something from the spec we can 
quote in the code?

>> Another question is whether the restriction would make sense. For example, 
>> does it ever make sense to specify both is_device_ptr and firstprivate for 
>> the same variable on a target construct?
> 
> On a `target` construct - no.

Why not?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

> I should have reported that the current implementation isn't complete for 
> OpenMP 4.5. For example, on target teams, reduction(+:x) map(x) is an error 
> but not map(x) reduction(+:x). So there are bugs to fix, and maybe this will 
> evolve into multiple patches, but I want to be sure I'm on the right path 
> first.

It is just a bug, not a missing feature. Just file a bug report for it.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:10895
+// combined construct.
+if (CurrDir == OMPD_target) {
   OpenMPClauseKind ConflictKind;

jdenny wrote:
> ABataev wrote:
> > I would suggest to guard this change and limit this new functionality only 
> > for OpenMP 5.0. 
> Do you agree that this is strictly an extension to 4.5 that won't alter the 
> behavior of 4.5-conforming applications?
> 
> Do we generally want to complain about the use of extensions, or is there 
> another reason for the guard you suggest?
No. It is incorrect according to OpenMP 4.5 and we shall emit diagnostics here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

> Maybe, but I haven't found any statement in either version that states that 
> map restrictions apply to is_device_ptr.

`is_device_ptr` is a kind of mapping clause. I assume we can extend the 
restrictions for `map` clause to this clause too.

> Another question is whether the restriction would make sense. For example, 
> does it ever make sense to specify both is_device_ptr and firstprivate for 
> the same variable on a target construct?

On a `target` construct - no. On a `target parallel` - yes. This may be 
important especially in unified shared memory mode.




Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > I think this should cause an error or at least a warning. Telling the 
> > > compiler `ps` is a device pointer only to create a local uninitialized 
> > > shadowing variable seems like an error to me.
> > It is allowed according to OpenMP 5.0. Private copy must be created in the 
> > context of the parallel region, not the target region. So, for OpenMP 5.0 
> > we should not emit error here.
> What does that mean and how does that affect my reasoning?
It means, that for OpenMP 5.0 we should emit a warning/error here. It is 
allowed according to the standard, we should allow it too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

ABataev wrote:
> jdoerfert wrote:
> > I think this should cause an error or at least a warning. Telling the 
> > compiler `ps` is a device pointer only to create a local uninitialized 
> > shadowing variable seems like an error to me.
> It is allowed according to OpenMP 5.0. Private copy must be created in the 
> context of the parallel region, not the target region. So, for OpenMP 5.0 we 
> should not emit error here.
What does that mean and how does that affect my reasoning?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

jdoerfert wrote:
> I think this should cause an error or at least a warning. Telling the 
> compiler `ps` is a device pointer only to create a local uninitialized 
> shadowing variable seems like an error to me.
It is allowed according to OpenMP 5.0. Private copy must be created in the 
context of the parallel region, not the target region. So, for OpenMP 5.0 we 
should not emit error here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added a comment.

In D65835#1618865 , @ABataev wrote:

> `is_device_ptr` can be considered as a kind of mapping clause (see 2.19.7   
> Data-Mapping Attribute Rules, Clauses, and Directives), so, I assume, clang 
> correct here in terms of OpenMP 4.5.


Maybe, but I haven't found any statement in either version that states that map 
restrictions apply to is_device_ptr.

Another question is whether the restriction would make sense.  For example, 
does it ever make sense to specify both is_device_ptr and firstprivate for the 
same variable on a target construct?  I think that would mean that 
modifications that are made to that device pointer within the target region 
should not be seen after the target region.  I think that's reasonable, but 
that combination is not possible with the restriction.  As Johannes points out, 
private plus is_device_ptr probably doesn't make sense.

> Thus, I would not call this a "fix", this is just a new feature from OpenMP 
> 5.0.

Understood.

I should have reported that the current implementation isn't complete for 
OpenMP 4.5.  For example, on `target teams`, `reduction(+:x) map(x)` is an 
error but not `map(x) reduction(+:x)`.  So there are bugs to fix, and maybe 
this will evolve into multiple patches, but I want to be sure I'm on the right 
path first.

> Plus, these changes are not enough to support this new feature from OpenMP 
> 5.0. There definitely must be some changes in the codegen. If the variable is 
> mapped in the target construct, we should not generate a code for the private 
> clause of this variable on the target construct, since, in this case, private 
> clauses are applied for the inner subdirectives, which are the part of the 
> combined directive, but not the `target` part of the directive.

I'll look into it.

Thanks for the quick review.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:10895
+// combined construct.
+if (CurrDir == OMPD_target) {
   OpenMPClauseKind ConflictKind;

ABataev wrote:
> I would suggest to guard this change and limit this new functionality only 
> for OpenMP 5.0. 
Do you agree that this is strictly an extension to 4.5 that won't alter the 
behavior of 4.5-conforming applications?

Do we generally want to complain about the use of extensions, or is there 
another reason for the guard you suggest?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp:93
   ;
-#pragma omp target parallel for private(ps) is_device_ptr(ps) // 
expected-error{{private variable cannot be in a is_device_ptr clause in 
'#pragma omp target parallel for' directive}} expected-note{{defined as 
private}}
+#pragma omp target parallel for private(ps) is_device_ptr(ps)
 for (int ii=0; ii<10; ii++)

I think this should cause an error or at least a warning. Telling the compiler 
`ps` is a device pointer only to create a local uninitialized shadowing 
variable seems like an error to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

`is_device_ptr` can be considered as a kind of mapping clause (see 2.19.7   
Data-Mapping Attribute Rules, Clauses, and Directives), so, I assume, clang 
correct here in terms of OpenMP 4.5.
Thus, I would not call this a "fix", this is just a new feature from OpenMP 5.0.
Plus, these changes are not enough to support this new feature from OpenMP 5.0. 
There definitely must be some changes in the codegen. If the variable is mapped 
in the target construct, we should not generate a code for the private clause 
of this variable on the target construct, since, in this case, private clauses 
are applied for the inner subdirectives, which are the part of the combined 
directive, but not the `target` part of the directive.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:10895
+// combined construct.
+if (CurrDir == OMPD_target) {
   OpenMPClauseKind ConflictKind;

I would suggest to guard this change and limit this new functionality only for 
OpenMP 5.0. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65835/new/

https://reviews.llvm.org/D65835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added a reviewer: ABataev.
Herald added a subscriber: guansong.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

For `map`, the following restriction changed in OpenMP 5.0:

- OpenMP 4.5 [2.15.5.1, Restrictions]: "A list item cannot appear in both a map 
clause and a data-sharing attribute clause on the same construct.

- OpenMP 5.0 [2.19.7.1, Restrictions]: "A list item cannot appear in both a map 
clause and a data-sharing attribute clause on the same construct unless the 
construct is a combined construct."

For `is_device_ptr`, I don't see a restriction in either version, but 
perhaps I missed it.  However, Clang implements a similar restriction
for `is_device_ptr` as for `map`.

This patch removes this restriction in the case of combined
constructs, both for `map` and for `is_device_ptr`.  It does not 
remove the restriction for non-combined constructs even though I'm not 
sure why `is_device_ptr` has the restriction at all.

I haven't yet added any new tests for the `is_device_ptr` case.  I'm 
happy to do so if reviewers feel this patch is heading in the right
direction.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65835

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_parallel_for_is_device_ptr_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_is_device_ptr_messages.cpp
  clang/test/OpenMP/target_parallel_is_device_ptr_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_is_device_ptr_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_lastprivate_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_is_device_ptr_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_lastprivate_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_firstprivate_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_is_device_ptr_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_lastprivate_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_private_messages.cpp
  clang/test/OpenMP/target_teams_is_device_ptr_messages.cpp
  clang/test/OpenMP/target_teams_map_codegen.cpp
  clang/test/OpenMP/target_teams_map_messages.cpp

Index: clang/test/OpenMP/target_teams_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_map_messages.cpp
+++ clang/test/OpenMP/target_teams_map_messages.cpp
@@ -536,10 +536,10 @@
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
 
-#pragma omp target teams private(j) map(j) // expected-error {{private variable cannot be in a map clause in '#pragma omp target teams' directive}}  expected-note {{defined as private}}
+#pragma omp target teams private(j) map(j)
   {}
 
-#pragma omp target teams firstprivate(j) map(j)  // expected-error {{firstprivate variable cannot be in a map clause in '#pragma omp target teams' directive}} expected-note {{defined as firstprivate}}
+#pragma omp target teams firstprivate(j) map(j)
   {}
 
 #pragma omp target teams map(m)
Index: clang/test/OpenMP/target_teams_map_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/target_teams_map_codegen.cpp
@@ -0,0 +1,64 @@
+// Test host codegen.
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -x c++ -triple i386-unknown-unknown -fopenmp-targets=i386-pc-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple i386-unknown-unknown -fopenmp-targets=i386-pc-linux-gnu -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -triple i386-unknown-unknown -fopenmp-targets=i386-pc-linux-gnu -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+
+// Test target codegen - host bc file has to be created first.
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm-bc %s -o %t-ppc-host.bc
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple