[PATCH] D56113: [OpenMP] Replace predetermined shared for const variable

Thu Jan 3 12:34:02 PST 2019

jdenny added a comment.

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?

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

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56113/new/

https://reviews.llvm.org/D56113