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

[Praveen G via Phabricator via llvm-commits]

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

@kiranchandramohan @SouraVX Thanks for the review ! I have addressed the review comments as specified.



================
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) {
----------------
SouraVX wrote:
> Not sure of this version specific comment, since we've not followed that consistently.
>  @kiranchandramohan @jdoerfert @kiranktp Please share your opinion.
Removed the version specific comment.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:898
   ResolveOmpObjectList(list, Symbol::Flag::OmpThreadprivate);
-  return false;
+  return true;
 }
----------------
SouraVX wrote:
> 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.
@SouraVX  Yes . I am resolving ThreadPrivate as it is required for Copyin . Updated the revision summary . Thanks!


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:226-239
+    return true;
+  }
+
+  bool Pre(const parser::OpenMPDeclarativeConstruct &x) {
+    std::visit(common::visitors{
+                   [&](const parser::OpenMPThreadprivate &construct) {
+                     if (Pre(construct))
----------------
kiranchandramohan wrote:
> Why is this change required?
@kiranchandramohan  This change is required to identify the list items present on the OpenMPThreadprivate directive as the semantic checks for the declarative directives are not yet implemented at present . 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:976
+                  CheckDataCopyingClause(*name, *symbol, ompFlag);
+                  return;
+                }
----------------
kiranchandramohan wrote:
> Nit: I think in general early returns are not used widely in the code base.
Modified to if - else block instead of early return


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89385