In D56541#1352955, @delcypher wrote:

Okay I get the idea. This is a much more invasive change however. Would it be okay to commit this (with FILECHECK_OPTS also stripped out) and do your suggestion as a follow up patch?
The reason I'd prefer to do this is because this change is intended to fix some internal build bots. Due to where we are in the release cycle the changes I make need to be simple (i.e. the smaller the patch the better) in order to be accepted by internal reviewers.

This patch does improve the situation for everyone, and it's easy to revert, so I'm fine with that. But could you fix lit's test suite too? Should be easy, right? I'm not aware of any other test suites that are sensitive to FileCheck's output in this manner.

If it's straight forward I'll land it in a separate patch.

Unless you plan to write the followup patch immediately, could you please add a bugzilla and cc me?

https://bugs.llvm.org/show_bug.cgi?id=40284

The summary needs to be updated for the extraction of D56549. Otherwise, LGTM. I've accepted, but others have time to comment while D56549 is reviewed.

I commented when this was part of D55940. However, I'm not as familiar with this area, especially the unit testing, so someone else should review further.

In D56541#1352784, @delcypher wrote:

In D56541#1352721, @jdenny wrote:

I've been wanting to fix this too. Thanks for working on it. A few issues:

• FILECHECK_OPTS should be cleared too.

Sure I can fix that one.

I've been wanting to fix this too. Thanks for working on it. A few issues:

• In dump-input-annotations.txt, use -implicit-check-not to thoroughly check for trace suppression.
• In dump-input-enable.txt, improve comments, and cover cases of -dump-input=never|fail without -v.

Thanks for reviewing!

Ping.

Thanks for the review!

Update for changes to earlier patches in series.

Update for changes to earlier patch in series.

Split checkConstNotMutableType implementation.

Update for changes to earlier patches in series.

Update for changes to earlier patch in series.

• Reuse new const-not-mutable check when computing predetermined shared attribute.
• Quote 3.1 spec to clarify that private and lastprivate has the const-not-mutable restriction there too.
• Don't add new test case shared_for_const_not_mutable.cpp, which r350127 addressed.

Rebased. Ping.

In D56113#1345529, @jdenny wrote:

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

• For OpenMP <= 3.1, add back predetermined shared for const variables without mutable fields.
• Quote the spec for calls to rejectConstNotMutableType for private and lastprivate.
• Update parallel_default_messages.cpp extension now that it depends on the OpenMP version.

In D56113#1345578, @ABataev wrote:

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.

In D56113#1345535, @ABataev wrote:

In D56113#1345529, @jdenny wrote:

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

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

In D56113#1345337, @ABataev wrote:

In D56113#1345238, @ABataev wrote:

In D56113#1345232, @jdenny wrote:

In D56113#1345047, @ABataev wrote:

In D56113#1344210, @jdenny wrote:

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

For reductions, this will change the message from "const-qualified list item cannot be reduction" to "const-qualified variable cannot be reduction". Is that OK? (There are many tests to update, so I want to ask first.)

Mmmm. For the reductions better to keep the original message, because the list items also might be the array sections, not only the variables.

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

In D56113#1345047, @ABataev wrote:

In D56113#1344210, @jdenny wrote:

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

Other than nits I've added as inline comments, LGTM. However, let's give Paul some time to respond now that the holidays are over.

Ping?

In D56113#1342879, @ABataev wrote:

But you will need another serie of patches for reduction and linear clauses to update their error messages.

• Apply reviewer suggestions.
• Add test case for change in default(none) behavior.

Hi Alexey,

In D56113#1341940, @ABataev wrote:

The patch does not seem quite correct. I committed another fix for this problem.

In D55940#1338920, @thopre wrote:

I've just given it a try but there seems to be some sorcery involved in that case: G contains just 10 instead of =10. Any way I could prevent the parsing from eating a leading equal sign? If not, I could reword the error message to mention leading equal sign but we would fail to diagnose an issue when passing -D'=Expression: VAR-5=10' typed by error instead of -D'TXT=Expression: VAR-5=10'.

In D55940#1337906, @probinson wrote:

This will permit -DVALUE= (with no value) is that a supported usage?

Better diagnostics would definitely be helpful here.

Apply variable renames suggested by grokos.

• Fix a comment typo.

Remaining suggestions done in D55738.

In D53897#1331556, @probinson wrote:

The terminology around "possible" "expected" and "final" matches probably warrants a few lines of commentary somewhere, unless I missed it in an earlier patch. There are getting to be enough cases that the casual reader (ahem) can't just Zen their way through them all.