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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

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 ?



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:744
 
+void OmpStructureChecker::CheckIntentInPointer(
+    const parser::OmpObjectList &objectList, const llvm::omp::Clause clause) {
----------------
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?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1182
+    // 'private' or 'firstprivate' clause on a single construct
+    if (IsObjectWithDSA(symbol)) {
+      context_.Say(name.source,
----------------
Will this test erroneously include DSA other than private, 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());
----------------
Should there be a check for the SINGLE construct here?


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


================
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.
+
----------------
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)


================
Comment at: flang/test/Semantics/omp-lastprivate02.f90:1
+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91920