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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
