This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Replace predetermined shared for const variable
ClosedPublic

Authored by jdenny on Dec 27 2018, 2:59 PM.

Details

Summary

The following appears in OpenMP 3.1 sec. 2.9.1.1 as a predetermined
data-sharing attribute:

Variables with const-qualified type having no mutable member are
shared.

It does not appear in OpenmP 4.0, 4.5, or 5.0. This patch removes the
implementation of that attribute when the requested OpenMP version is
greater than 3.1.

One effect of that removal is that default(none) affects const
variables without mutable members.

Also, without this patch, if a const variable without mutable members
was explicitly lastprivate or private, it was an error because it was
predetermined shared. Now, clang instead complains that it's const
without mutable fields, which is a more intelligible diagnostic. That
should be fine for all of the above versions because they all have
something like the following, which is quoted from OpenMP 5.0
sec. 2.19.3:

A variable that is privatized must not have a const-qualified type
unless it is of class type with a mutable member. This restriction does
not apply to the firstprivate clause.

reduction and linear clauses already have separate checks for const
variables. Future patches will merge the implementations.

Diff Detail

Repository
rC Clang

Event Timeline

jdenny created this revision.Dec 27 2018, 2:59 PM

The patch does not seem quite correct. I committed another fix for this problem.
There is still the problem with the explicitly specified shared clause on target teams directive, but I don't think that it must cause some troubles. The variable still should not be transferred from device to the host, so it is fine to pass by value into inner teams regions. Though, generally speaking, it must be fixed.

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

Thanks for the fix, r350127, which does seem to address the use cases I care about right now.

However, you are still implementing predetermined shared for const variables. Assuming OpenMP 4.0 or later, that produces misleading diagnostics, such as "error: shared variable cannot be lastprivate". Moreover, default(none) doesn't have the expected effect on such variables. Then again, neither of these issues seems like a severe problem, and your approach is more compliant with earlier OpenMP versions. Does all that match your view?

There is still the problem with the explicitly specified shared clause on target teams directive, but I don't think that it must cause some troubles. The variable still should not be transferred from device to the host, so it is fine to pass by value into inner teams regions. Though, generally speaking, it must be fixed.

Should there be a copy allocated on the device and shared among the teams? By the way, removing const from the example doesn't avoid this bug, but splitting omp target teams into two directives does avoid it.

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

Thanks for the fix, r350127, which does seem to address the use cases I care about right now.

However, you are still implementing predetermined shared for const variables. Assuming OpenMP 4.0 or later, that produces misleading diagnostics, such as "error: shared variable cannot be lastprivate". Moreover, default(none) doesn't have the expected effect on such variables. Then again, neither of these issues seems like a severe problem, and your approach is more compliant with earlier OpenMP versions. Does all that match your view?

There is still the problem with the explicitly specified shared clause on target teams directive, but I don't think that it must cause some troubles. The variable still should not be transferred from device to the host, so it is fine to pass by value into inner teams regions. Though, generally speaking, it must be fixed.

Should there be a copy allocated on the device and shared among the teams? By the way, removing const from the example doesn't avoid this bug, but splitting omp target teams into two directives does avoid it.

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

Thanks for the fix, r350127, which does seem to address the use cases I care about right now.

However, you are still implementing predetermined shared for const variables. Assuming OpenMP 4.0 or later, that produces misleading diagnostics, such as "error: shared variable cannot be lastprivate". Moreover, default(none) doesn't have the expected effect on such variables. Then again, neither of these issues seems like a severe problem, and your approach is more compliant with earlier OpenMP versions. Does all that match your view?

I think this is a different problem, which you can try to fix. You just need to remove the rule for the const variables and carefully update all the checks in clauses. Also, you will need to add special checks for the constant types in all private clauses.
If you want, you can implement this.Also, please note, that this going to be a serie of the patch - the first with the predetermined rule removal and several others with the correct handling of the constant types in the private clauses.

There is still the problem with the explicitly specified shared clause on target teams directive, but I don't think that it must cause some troubles. The variable still should not be transferred from device to the host, so it is fine to pass by value into inner teams regions. Though, generally speaking, it must be fixed.

Should there be a copy allocated on the device and shared among the teams? By the way, removing const from the example doesn't avoid this bug, but splitting omp target teams into two directives does avoid it.

Just like I said, this is not a real problem. Moreover, it saves us one register for one extra load operation. So, I don't think this is a real problem and really requires a fix.

Hi Alexey,

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

Thanks for the fix, r350127, which does seem to address the use cases I care about right now.

However, you are still implementing predetermined shared for const variables. Assuming OpenMP 4.0 or later, that produces misleading diagnostics, such as "error: shared variable cannot be lastprivate". Moreover, default(none) doesn't have the expected effect on such variables. Then again, neither of these issues seems like a severe problem, and your approach is more compliant with earlier OpenMP versions. Does all that match your view?

I think this is a different problem, which you can try to fix. You just need to remove the rule for the const variables and carefully update all the checks in clauses. Also, you will need to add special checks for the constant types in all private clauses.
If you want, you can implement this.

I thought that's what I did in the patch I submitted. Would you please get me started by showing me an example where it doesn't behave correctly? I might be able to find remaining cases from that.

Also, please note, that this going to be a serie of the patch - the first with the predetermined rule removal and several others with the correct handling of the constant types in the private clauses.

I'm happy to break up the patch. However, the first patch you describe would leave the test suite failing (many diagnostics will be skipped), and the remaining patches you describe would fix the test suite. Is that ok?

There is still the problem with the explicitly specified shared clause on target teams directive, but I don't think that it must cause some troubles. The variable still should not be transferred from device to the host, so it is fine to pass by value into inner teams regions. Though, generally speaking, it must be fixed.

Should there be a copy allocated on the device and shared among the teams? By the way, removing const from the example doesn't avoid this bug, but splitting omp target teams into two directives does avoid it.

Just like I said, this is not a real problem. Moreover, it saves us one register for one extra load operation. So, I don't think this is a real problem and really requires a fix.

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?

Thanks.

Hi Alexey,

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

Thanks for the fix, r350127, which does seem to address the use cases I care about right now.

However, you are still implementing predetermined shared for const variables. Assuming OpenMP 4.0 or later, that produces misleading diagnostics, such as "error: shared variable cannot be lastprivate". Moreover, default(none) doesn't have the expected effect on such variables. Then again, neither of these issues seems like a severe problem, and your approach is more compliant with earlier OpenMP versions. Does all that match your view?

I think this is a different problem, which you can try to fix. You just need to remove the rule for the const variables and carefully update all the checks in clauses. Also, you will need to add special checks for the constant types in all private clauses.
If you want, you can implement this.

I thought that's what I did in the patch I submitted. Would you please get me started by showing me an example where it doesn't behave correctly? I might be able to find remaining cases from that.

I thought you kept the original message, just missed that. The original cause of the problem was a little bit different and it masked the real problem. You can go ahead with your patch, update it and resend for the review.

Also, please note, that this going to be a serie of the patch - the first with the predetermined rule removal and several others with the correct handling of the constant types in the private clauses.

I'm happy to break up the patch. However, the first patch you describe would leave the test suite failing (many diagnostics will be skipped), and the remaining patches you describe would fix the test suite. Is that ok?

Ok, let's not break it. But you will need another serie of patches for reduction and linear clauses to update their error messages.

There is still the problem with the explicitly specified shared clause on target teams directive, but I don't think that it must cause some troubles. The variable still should not be transferred from device to the host, so it is fine to pass by value into inner teams regions. Though, generally speaking, it must be fixed.

Should there be a copy allocated on the device and shared among the teams? By the way, removing const from the example doesn't avoid this bug, but splitting omp target teams into two directives does avoid it.

Just like I said, this is not a real problem. Moreover, it saves us one register for one extra load operation. So, I don't think this is a real problem and really requires a fix.

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.

Thanks.

Hi Alexey,

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

Thanks for the fix, r350127, which does seem to address the use cases I care about right now.

However, you are still implementing predetermined shared for const variables. Assuming OpenMP 4.0 or later, that produces misleading diagnostics, such as "error: shared variable cannot be lastprivate". Moreover, default(none) doesn't have the expected effect on such variables. Then again, neither of these issues seems like a severe problem, and your approach is more compliant with earlier OpenMP versions. Does all that match your view?

I think this is a different problem, which you can try to fix. You just need to remove the rule for the const variables and carefully update all the checks in clauses. Also, you will need to add special checks for the constant types in all private clauses.
If you want, you can implement this.

I thought that's what I did in the patch I submitted. Would you please get me started by showing me an example where it doesn't behave correctly? I might be able to find remaining cases from that.

Also, please note, that this going to be a serie of the patch - the first with the predetermined rule removal and several others with the correct handling of the constant types in the private clauses.

I'm happy to break up the patch. However, the first patch you describe would leave the test suite failing (many diagnostics will be skipped), and the remaining patches you describe would fix the test suite. Is that ok?

There is still the problem with the explicitly specified shared clause on target teams directive, but I don't think that it must cause some troubles. The variable still should not be transferred from device to the host, so it is fine to pass by value into inner teams regions. Though, generally speaking, it must be fixed.

Should there be a copy allocated on the device and shared among the teams? By the way, removing const from the example doesn't avoid this bug, but splitting omp target teams into two directives does avoid it.

Just like I said, this is not a real problem. Moreover, it saves us one register for one extra load operation. So, I don't think this is a real problem and really requires a fix.

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?

Thanks.

ABataev added inline comments.Dec 31 2018, 10:17 AM
clang/lib/Sema/SemaOpenMP.cpp
9702 ↗(On Diff #179586)

Functions must start from the small letters.

9716 ↗(On Diff #179586)

I would check here for RD rather than CPlusPlus. int types cannot have mutable fields, for example.

jdenny updated this revision to Diff 179935.Jan 2 2019, 1:49 PM
jdenny edited the summary of this revision. (Show Details)
  • Apply reviewer suggestions.
  • Add test case for change in default(none) behavior.
jdenny marked 2 inline comments as done.Jan 2 2019, 1:50 PM

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.

By the way, is there any value to keeping the predetermined shared for const if -openmp-version=3.1 or earlier?

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?

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.

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.

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.

jdenny added a comment.Jan 3 2019, 8:36 AM

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?

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.

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.

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.

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.

Ah, for some reason I assumed it was a string not an integer.

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

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.

Ah, for some reason I assumed it was a string not an integer.

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.

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.

I need to add a parameter to rejectConstNotMutableType to specify whether decl/def notes are included, and I can reuse that parameter for this purpose. Whether the diagnostic says "list item" or "variable" will then depend on what the list item is (variable, array section, etc.) rather than what the clause is (private, reduction, etc.). I think that's ok.

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.

I need to add a parameter to rejectConstNotMutableType to specify whether decl/def notes are included, and I can reuse that parameter for this purpose. Whether the diagnostic says "list item" or "variable" will then depend on what the list item is (variable, array section, etc.) rather than what the clause is (private, reduction, etc.). I think that's ok.

Ok, 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.

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.

By the way, LangOpts.OpenMP currently defaults to 0. Should it?

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.

By the way, LangOpts.OpenMP currently defaults to 0. Should it?

Yes, it means it is disabled by default.

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.

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.

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 DSAStackTy::getTopDSA in SemaOpenMP.cpp, I added this line:

llvm::errs() << SemaRef.LangOpts.OpenMP << '\n';

clang -fopenmp test.c prints 1.

I'm at r350238, which I pulled yesterday.

For my purposes, 1 and 31 are equivalent, so I can move on.

If -fopenmp-targets= is used, then the default version is 45.

That I can confirm.

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 DSAStackTy::getTopDSA in SemaOpenMP.cpp, I added this line:

llvm::errs() << SemaRef.LangOpts.OpenMP << '\n';

clang -fopenmp test.c prints 1.

I'm at r350238, which I pulled yesterday.

For my purposes, 1 and 31 are equivalent, so I can move on.

Ah, yes, forgot already. You can consider 1 as the same value as 31 by default.

If -fopenmp-targets= is used, then the default version is 45.

That I can confirm.

jdenny updated this revision to Diff 180139.Jan 3 2019, 2:20 PM
jdenny edited the summary of this revision. (Show Details)
jdenny set the repository for this revision to rC Clang.
  • 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.

Changes for reduction and linear will come in later patches.

jdenny edited the summary of this revision. (Show Details)Jan 3 2019, 2:21 PM

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.

I missed something: 3.1 restricts const and not mutable variables for lastprivate and private too. However, clang didn't previously implement those restrictions probably because the check for conflicts with predetermined shared was sufficient to catch those cases.

So, for 3.1, it should be correct for clang to report either diagnostic, but again I think the new diagnostic is clearer. For 4.0 and later, only the new diagnostic makes sense.

I had already updated all the tests for the new diagnostic, so it's easiest to keep the new diagnostic for all versions. Moreover, it means less cases to check in the test suite.

Of course, I can adjust if this doesn't make sense to you.

ABataev added inline comments.Jan 4 2019, 6:27 AM
clang/lib/Sema/SemaOpenMP.cpp
9736 ↗(On Diff #180139)

This function and the original code has a lot of common code. Could you, please, use this function somehow for the original too? Or outline the common code into standalone function

ABataev added inline comments.Jan 4 2019, 6:33 AM
clang/lib/Sema/SemaOpenMP.cpp
9736 ↗(On Diff #180139)

Also, probably missed the check if the OpenMP version is >= 4.0

jdenny marked an inline comment as done.Jan 4 2019, 7:49 AM
jdenny added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
9736 ↗(On Diff #180139)

This function and the original code has a lot of common code. Could you, please, use this function somehow for the original too? Or outline the common code into standalone function

Sure, I'll work on that.

Also, probably missed the check if the OpenMP version is >= 4.0

This is what I was trying to explain in my last comment: I now realize that 3.1 also has the const restriction for private and lastprivate. For example:

A variable that appears in a private clause must not have a const-qualified type
unless it is of class type with a mutable member.

To make it clear that this isn't limited to 4.0 and later, perhaps I should also quote 3.1 at the calls to rejectConstNotMutableType.

However, clang didn't previously implement those restrictions for private and lastprivate probably because the check for conflicts with predetermined shared was sufficient to catch those cases.

So, for 3.1, it should be correct for clang to report either diagnostic, but again I think the new diagnostic is clearer. For 4.0 and later, only the new diagnostic makes sense. So, my patch makes clang report the new diagnostic for all OpenMP versions.

Of course, I can adjust if this doesn't make sense to you.

jdenny updated this revision to Diff 180278.Jan 4 2019, 11:03 AM
  • 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.
ABataev added inline comments.Jan 4 2019, 11:08 AM
clang/lib/Sema/SemaOpenMP.cpp
1115 ↗(On Diff #180278)

I would say it is better to outlined check as a standalone function rather than use checkConstNotMutableType for this purpose

jdenny updated this revision to Diff 180299.Jan 4 2019, 12:23 PM
jdenny marked an inline comment as done.
jdenny set the repository for this revision to rC Clang.

Split checkConstNotMutableType implementation.

jdenny marked 3 inline comments as done.Jan 4 2019, 12:28 PM
This revision is now accepted and ready to land.Jan 4 2019, 12:33 PM

Thanks for the review!