[PATCH] D91920: [Flang] [OpenMP] Add semantic checks for OpenMP firstprivate , lastprivate and copyprivate clauses

[Praveen G via Phabricator via llvm-commits]

praveen marked 6 inline comments as done.
praveen added a comment.

In D91920#2410307 <https://reviews.llvm.org/D91920#2410307>, @kiranchandramohan wrote:

> In D91920#2410202 <https://reviews.llvm.org/D91920#2410202>, @praveen wrote:
>
>> @kiranchandramohan  In the test file //omp-clause-validity01.f90// , the following no symbol found errors are not being thrown.
>>
>>   !ERROR: Internal: no symbol found for 'i'
>>   !$omp taskwait depend(sink:i-1)
>>   
>>   !ERROR: Internal: no symbol found for 'first'
>>   !$omp critical (first)
>>
>> Since the errors related to copyprivate clause in omp-clause-validity01.f90 (line numbers 324 and 340 above)  being thrown as part of the checks for //**copyprivate in resolve-directives.cpp**// are marked as //**fatal **//errors , the flag //**errorOnUnresolvedName_**// is being set to **false** and the no symbol found error is not being thrown.
>>
>>    RewriteMutator(SemanticsContext &context)
>>    : errorOnUnresolvedName_{!context.AnyFatalError()},
>>       messages_{context.messages()} {}
>>   
>>   void RewriteMutator::Post(parser::Name &name) {
>>      if (!name.symbol && errorOnUnresolvedName_) {
>>        messages_.Say(name.source, "Internal: no symbol found for '%s'"_err_en_US,
>>          name.source);
>>       }
>>    }
>>
>> Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?
>>
>> Should all the checks related to copyprivate be moved to check-omp-structure.cpp?
>>
>> Can you please suggest the approach to follow for these changes?
>>
>> Thanks!
>
> I think these checks can stay in the current file since they need to check the data sharing attributes.
> For the depend clause I think the test need to be changed to a valid test. Is that OK @kiranktp.
> For critical I believe we may not have done the semantic check and hence the symbol is not available. For this I think it is OK to comment out the check.
>
>> Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?
>
> Is this OK @clementval ?

@kiranchandramohan @kiranktp @clementval I have commented the test cases which were failing with "no symbol found" error. 
The symbol in the sink of the depend clause (OmpDependSinkVec) is not resolved . Hence we are running into "no symbol found" error . Do we resolve it as part of a separate patch or can I include it as part of this patch?



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:744
 
+void OmpStructureChecker::CheckIntentInPointer(
+    const parser::OmpObjectList &objectList, const llvm::omp::Clause clause) {
----------------
kiranchandramohan wrote:
> Is this function part of this patch? If not can you either submit the other patch which introduces this code or make this patch a child of the other patch?
@kiranchandramohan This function was originally introduced in the patch for private clause https://reviews.llvm.org/D90210 .

Since it is not yet checked in and the same check is required for copyprivate clause, i have included this as part of this patch as well.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:744
 
+void OmpStructureChecker::CheckIntentInPointer(
+    const parser::OmpObjectList &objectList, const llvm::omp::Clause clause) {
----------------
praveen wrote:
> kiranchandramohan wrote:
> > Is this function part of this patch? If not can you either submit the other patch which introduces this code or make this patch a child of the other patch?
> @kiranchandramohan This function was originally introduced in the patch for private clause https://reviews.llvm.org/D90210 .
> 
> Since it is not yet checked in and the same check is required for copyprivate clause, i have included this as part of this patch as well.
This check is now merged to master .


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1182
+    // 'private' or 'firstprivate' clause on a single construct
+    if (IsObjectWithDSA(symbol)) {
+      context_.Say(name.source,
----------------
kiranchandramohan wrote:
> Will this test erroneously include DSA other than private, firstprivate?
@kiranchandramohan  Thanks for pointing this out . Added the checks for private and firstprivate.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1185
+          "COPYPRIVATE variable '%s' may not appear on a PRIVATE or "
+          "FIRSTPRIVATE clause on a SINGLE construct"_err_en_US,
+          symbol.name());
----------------
kiranchandramohan wrote:
> Should there be a check for the SINGLE construct here?
@kiranchandramohan Added the SINGLE construct check . Thanks


================
Comment at: flang/test/Semantics/omp-firstprivate01.f90:1
+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
----------------
kiranchandramohan wrote:
> Can similar tests be all put in the same file? Like all tests which test for  2.15.3.4.
@kiranchandramohan Combined all the similar test cases into a single file.


================
Comment at: flang/test/Semantics/omp-lastprivate01.f90:4
+! 2.15.3.5 lastprivate Clause
+! A variable that appears in a lastprivate clause must be definable.
+
----------------
kiranchandramohan wrote:
> Can tests be added for the following cases. If it is not covered elsewhere?
> -> module variable which is protected.
> -> subroutine argument which is intent(in)
@kiranchandramohan Added test cases for the module variable which is protected and subroutine argument which is intent(in).


================
Comment at: flang/test/Semantics/omp-lastprivate02.f90:1
+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
----------------
kiranchandramohan wrote:
> Combine this test and the next one if they are testing the same thing for different worksharing constructs.
> Add a test for single as well if possible.
@kiranchandramohan Combined the test cases as suggested . 
Lastprivate clause is not allowed on the single clause. 


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

https://reviews.llvm.org/D91920