[PATCH] D89385: [Flang][OpenMP 4.5] Add semantic check for OpenMP copyin clause

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX added subscribers: kiranktp, SouraVX.
SouraVX added inline comments.


================
Comment at: flang/include/flang/Semantics/symbol.h:504
       OmpMapTo, OmpMapFrom, OmpMapAlloc, OmpMapRelease, OmpMapDelete,
+      // OpenMP data-copying attributes
+      OmpCopyIn,
----------------
NIT: `attribute` ?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:239
+               },
+        x.u);
     return false;
----------------
Please refer to comment in `flang/lib/Semantics/resolve-directives.cpp:898` below


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:287
 
+  // OpenMP 4.5 - 2.15.4.1 Copyin Clause
+  bool Pre(const parser::OmpClause::Copyin &x) {
----------------
Not sure of this version specific comment, since we've not followed that consistently.
 @kiranchandramohan @jdoerfert @kiranktp Please share your opinion.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:898
   ResolveOmpObjectList(list, Symbol::Flag::OmpThreadprivate);
-  return false;
+  return true;
 }
----------------
It seems to me that you're resolving `ThreadPrivate` too ? (Since `CopyIn` requires `ThreadPrivate` in first place).
Could you please modify the revision summary to reflect the intent.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:952
   }
+  if (auto *curr{
+          name ? GetContext().scope.FindCommonBlock(name->source) : nullptr}) {
----------------
Can we have a better name to convey the intent ?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:977
+                } else {
+                  AddToContextObjectWithDSA(*symbol, ompFlag);
+                  if (dataSharingAttributeFlags.test(ompFlag)) {
----------------
This seems to be un-intentional formatting introduced by your editor. Could you please correct this ?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1001
+              if (!dataCopyingAttributeFlags.test(ompFlag)) {
+                CheckMultipleAppearances(
+                    name, *symbol, Symbol::Flag::OmpCommonBlock);
----------------
NIT: Formatting ?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1123
+
+    // List item that appears on copyin clause must be threadprivate
+    if (!checkSymbol->test(Symbol::Flag::OmpThreadprivate))
----------------
NIT: How about re-phrasing this as:
```
List of items/objects that can appear in an `copyin` clause must be `threadprivate`.
```


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

https://reviews.llvm.org/D89385