jdenny marked 2 inline comments as done. jdenny added a comment. In D65835#1619560 <https://reviews.llvm.org/D65835#1619560>, @ABataev wrote:
> In D65835#1619549 <https://reviews.llvm.org/D65835#1619549>, @jdenny wrote: > > > In D65835#1619526 <https://reviews.llvm.org/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 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. 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
