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 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.
> >
> Thanks for the discussion. It sounds like people are fine if (1) the
> diagnostic proposed here would be a warning not an error and (2) that warning
> would not be implemented by this patch.
yes
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
