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

Thu Aug 8 06:15:37 PDT 2019

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 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 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.
> > 
> > There are at least two relevant factors:
> > 
> >  1. Language standards often express general concepts that can be combined in some regular set of ways. Some of these combinations are likely unintentional (e.g., user error), but standards don't explicitly prohibit them because: a) standards committees have limited bandwidth and need to concentrate on the highest-priority items and new features b) filling standards with a large number of special cases, even in the name of preventing user error, itself has a cost (in terms of maintenance of the standard, constraining conforming implementation techniques, and so on).
> > 
> >  2. Even if a standards committee were to take up restricting some set of special cases, implementation experience with a warning is often very helpful. Saying, "we added a warning, and no one complained about it being a false positive" is good evidence in support of making that warning a mandated error.
> > 
> > In the end, standards committees depend on implementers to add value on top of the standard itself in creating an high-QoI products. This has always been a focus area of Clang, and Clang is well known for its high diagnostic quality - not just in error messages, but in warnings too.
> > 
> > I have plenty of users who specifically compile with multiple compilers specifically to get the warnings for each compiler. Is it sometimes true that some compilers generating some warnings ends up being problematic? yes. I think that we all have observed that. But warnings are very helpful in catching likely bugs, and implementations have more freedom with warnings than with errors, so many users depend on high-quality warnings to help quickly find bugs and, thus, increase their productivity.
> > 
> Just like I said, if you think there are some incorrect combinations we could generate a warning. But better to implement it in a different patch. There are many possible combinations and each one may have different preconditions.
I have no objection to adding warnings in separate patch. I simply wanted to provide some feedback on the general conditions under which we should consider adding warnings. Thanks, Alexey.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65835