[PATCH] D90210: [flang] [OpenMP 4.5] Add semantic checks for OpenMP Private clause

[Praveen G via Phabricator via llvm-commits]

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

@kiranchandramohan Updated the reply for each of the inline comments. 
Thanks!



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:413-431
+  for (const auto &ompObject : x.v.v) {
+    if (const auto *name{parser::Unwrap<parser::Name>(ompObject)}) {
+      if (const auto *symbol{name->symbol}) {
+        if (const auto *commonBlockDetails{
+                symbol->detailsIf<CommonBlockDetails>()}) {
+          for (const auto &object : commonBlockDetails->objects()) {
+            CheckIntentInPointer(
----------------
sameeranjoshi wrote:
> Is it possible to extract this into a function and make it more generalized ?
> I see copyprivate in OMP 4.5 standard restriction section point 10 in [2.15.4.2] too has a similar restriction, we can probably reuse it over there.
> 
Moved the code to a function as suggested . Thanks !


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1054
+                  if (privateDataSharingAttributeFlags.test(ompFlag)) {
+                    CheckObjectInNamelist(*name, *symbol, ompFlag);
+                  }
----------------
kiranchandramohan wrote:
> "Variables that appear in namelist statements, in variable format expressions, and in expressions for statement function definitions, may not appear in a private clause."
> 
> The specific restriction that this code checks seems to have two more components. Will the checks for "in variable format expressions, and in expressions for statement function definitions," be in a separate patch?
@kiranchandramohan The checks for "Variable format expressions and in expressions for statement function definitions" will be handled in a separate patch.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1228
+      context_.Say(name.source,
+          "Variable '%s' in %s clause is used in NAMELIST statement"_err_en_US,
+          name.ToString(), clauseName.str());
----------------
kiranchandramohan wrote:
> Can this error message be like, Variable in namelist %s cannot be in a ^s clause?
Modified the error message. Thanks!


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1087
+    const auto *target{&symbol};
+    if (ompFlagsRequireNewSymbol.test(ompFlag)) {
+      if (const auto *details{symbol.detailsIf<HostAssocDetails>()}) {
----------------
kiranchandramohan wrote:
> why is this check required?
Removed the redundant check.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1092
+    }
+    auto it{std::find(namelistSymbols.begin(), namelistSymbols.end(), *target)};
+    if (it != namelistSymbols.end()) {
----------------
kiranchandramohan wrote:
> Can you check what is happening for this testcase here or above and why the error is being missed?
> 
> ```
> program omp_target
>   integer :: a, b, c
>   namelist /mylist/ a, b, c
> 
>   a = 5
>   b = 10
>   call sb()
> contains
>   subroutine sb()
>   !$omp parallel private(a)
>   c = a+b
>   !$omp end parallel
> 
>   write(*, mylist)
>   end subroutine
> end program omp_target
> ```
@kiranchandramohan Thanks for pointing this out . Since only the namelist variables in the enclosing scope are considered , this error is getting missed out. 

Working on modifying the code to get this working for all possible cases.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1092
+    }
+    auto it{std::find(namelistSymbols.begin(), namelistSymbols.end(), *target)};
+    if (it != namelistSymbols.end()) {
----------------
praveen wrote:
> kiranchandramohan wrote:
> > Can you check what is happening for this testcase here or above and why the error is being missed?
> > 
> > ```
> > program omp_target
> >   integer :: a, b, c
> >   namelist /mylist/ a, b, c
> > 
> >   a = 5
> >   b = 10
> >   call sb()
> > contains
> >   subroutine sb()
> >   !$omp parallel private(a)
> >   c = a+b
> >   !$omp end parallel
> > 
> >   write(*, mylist)
> >   end subroutine
> > end program omp_target
> > ```
> @kiranchandramohan Thanks for pointing this out . Since only the namelist variables in the enclosing scope are considered , this error is getting missed out. 
> 
> Working on modifying the code to get this working for all possible cases.
@kiranchandramohan Modified the logic to identify all the namelist variables defined in different scopes and added the test cases for the same. 

Thanks!


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1094
+    if (it != namelistSymbols.end()) {
+      llvm::StringRef clauseName = "PRIVATE";
+      if (ompFlag == Symbol::Flag::OmpFirstPrivate)
----------------
kiranchandramohan wrote:
> Nit: Use braced initializer here.
Made the change as suggested.


================
Comment at: flang/test/Semantics/omp-private01.f90:21
+
+  !ERROR: Pointer 'q' in the COMMON block with the INTENT(IN) attribute may not appear in a PRIVATE clause
+  !$omp parallel private(/cmn/)
----------------
kiranchandramohan wrote:
> Is this check required given that dummy arguments can never be part of a common block?
Removed the redundant check as suggested. Thanks!


================
Comment at: flang/test/Semantics/omp-private02.f90:1
+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
----------------
kiranchandramohan wrote:
> Can we move omp-private03.f90,  omp-private04.f90also into this file?
Moved the test cases in omp-private03.f90 and omp-private04.f90 to omp-private02.f90


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

https://reviews.llvm.org/D90210