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

Mon Dec 31 10:13:55 PST 2018

ABataev added a comment.

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

> Hi Alexey,
>
> > 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.
>
> 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.

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

> Hi Alexey,
>
> > 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.
>
> 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.

Repository:
  rC Clang

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

https://reviews.llvm.org/D56113