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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D56113#1342076 <https://reviews.llvm.org/D56113#1342076>, @jdenny wrote:

> In D56113#1341940 <https://reviews.llvm.org/D56113#1341940>, @ABataev wrote:
>
> > 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.




In D56113#1342076 <https://reviews.llvm.org/D56113#1342076>, @jdenny wrote:

> In D56113#1341940 <https://reviews.llvm.org/D56113#1341940>, @ABataev wrote:
>
> > 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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56113