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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
