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