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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

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

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




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

https://reviews.llvm.org/D56113