ABataev added a comment. In D56113#1345564 <https://reviews.llvm.org/D56113#1345564>, @jdenny wrote:
> In D56113#1345535 <https://reviews.llvm.org/D56113#1345535>, @ABataev wrote: > > > In D56113#1345529 <https://reviews.llvm.org/D56113#1345529>, @jdenny wrote: > > > > > In D56113#1345238 <https://reviews.llvm.org/D56113#1345238>, @ABataev > > > wrote: > > > > > > > >>> 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. > > > > > > > > > I'm planning to let this affect the behavior of `default(none)` > > > (predetermined shared means no explicit attribute is needed). > > > > > > I don't plan to let it affect which version of the diagnostics are > > > produced. I think the newer diagnostics are clearer even though they are > > > not expressed precisely in terms of 3.1 semantics. Moreover, there are > > > less cases to test this way. Let me know if you think this is wrong. If > > > you want to review the updated patch first, I'll be posting it soon. > > > > > > It would be good if could keep the original implementation for 3.1. > > > Should I parameterize all tests affected by this change to test both sets of > diagnostics? Or should we pick one version and just add a few additional > tests for the other? It is enough just to add the new test for the new version, no need to copy/update all the tests. > (I'm sorry to ask so many questions, but again there are many tests to > update.) > >>> By the way, LangOpts.OpenMP currently defaults to 0. Should it? >> >> Yes, it means it is disabled by default. > > Ah, it becomes 1 when we have `-fopenmp` but not `-fopenmp-version`. But > still `LangOpts.OpenMP <= 31` is true by default, so the new behavior would > be off by default. No, by default it gets value 31. If -fopenmp-targets= is used, then the default version is 45. And yes, you can use -fopenmp-version to set the required version of OpenMP. 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
