ABataev added a comment. In D56113#1345232 <https://reviews.llvm.org/D56113#1345232>, @jdenny wrote:
> In D56113#1345047 <https://reviews.llvm.org/D56113#1345047>, @ABataev wrote: > > > In D56113#1344210 <https://reviews.llvm.org/D56113#1344210>, @jdenny wrote: > > > > > In D56113#1342879 <https://reviews.llvm.org/D56113#1342879>, @ABataev > > > wrote: > > > > > > > But you will need another serie of patches for `reduction` and `linear` > > > > clauses to update their error messages. > > > > > > > > > Those already had their own checks for const. I'm not aware of any cases > > > not handled, and the test suite does pass after my patch. > > > > > > Yes, they have the checks for constness, but you need to update those > > checks to emit this new error message for const values with mutable fields. > > > I believe you mean we should reuse `rejectConstNotMutableType` for > `reduction` and `linear` rather than leave some of its implementation > duplicated for those. > > That is, I believe we should //not// relax the existing checks for > `reduction` and `linear` so that they permit const values with mutable > fields. First, that doesn't seem possible for `linear`, which requires > integral or pointer type. Second, that seems wrong for `reduction` because, > for whatever reason, OpenMP 5.0 sec. 2.19.5.1 page 298 says: > > A list item that appears in a reduction clause must not be const-qualified. > > > For that case, I can extend `rejectConstNotMutableType` with a bool parameter > to indicate mutable fields are not permitted. > > Does all that make sense? Yes, sounds good. > > >> >> >>> By the way, is there any value to keeping the predetermined shared for >>> const if -openmp-version=3.1 or earlier? >> >> Yes, you can check for the value of `LangOpts.OpenMP`. For OpenMP 3.1 it >> will have the value `31`. > > How far back should we take this? I'm inclined to check for `30` and `31` > only and assume anything else is newer, but let me know if we need to check > for earlier versions. I think `<= 31` is good. Clang always supported only OpenMP 3.1 and higher. > > >> >> >>>>> Sorry, I misunderstood your first response. When I try an atomic write >>>>> to the shared variable, the difference between the uncombined and >>>>> combined target teams directive affects functionality when targeting >>>>> multicore. Does that matter? >>>> >>>> Yes, I'm aware if this problem. But I don't think it is very important and >>>> can cause serious problems in user code. If you think that it is worth to >>>> fix this prblem, you can go ahead and fix it. >>> >>> No, not worthwhile for me right now. I just wanted to point it out in >>> passing in case it might matter to someone. Does a bugzilla seem >>> worthwhile to you? >> >> Yes, generally speaking it would be good to fix this. So, yes, at least to >> keep tracking, add a bug report to the bugzilla. > > Will do. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56113/new/ https://reviews.llvm.org/D56113 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
