[PATCH] D110714: [Flang][openmp] Added semantic checks for atomic construct

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Thanks for this patch. I have some suggestions, please have a look and let me know what you think.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:41
+// OpenMPAtomicConstruct
+class OmpAtomicConstructChecker {
+public:
----------------
clementval wrote:
> Might be cleaner in its own file?
I think this class and associated walk is probably not required. Additional walks and its instantiation adds to compilation time and should be used only as a last resort.

In the Atomic update/Atomic case, the AssignmentStatement can be obtained directly from the tuple since it is a part of the tuple inside the struct OmpAtomicUpdate or struct OmpAtomic nodes. Once you can extract the assignment, you can perform the checks that are done here inside the OmpStructureChecker class itself.

```
// ATOMIC UPDATE
struct OmpAtomicUpdate {
  ...
  std::tuple<OmpAtomicClauseList, Verbatim, OmpAtomicClauseList,
      Statement<AssignmentStmt>, std::optional<OmpEndAtomic>> t;
};
```


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:54
+            [&](const common::Indirection<parser::FunctionReference> &x) {
+              if (!CheckProcedureDesignatorValidity(x)) {
+                context_.Say(expr.source,
----------------
We have some similar code in reduction checking. Can you check the following links and see whether it will be beneficial to write CheckProcedureDesignatorValidity and CheckOperatorValidity in a similar way?

https://github.com/llvm/llvm-project/blob/28981015526f2192440c18f18e8a20cd11b0779c/flang/lib/Semantics/check-omp-structure.cpp#L1415
https://github.com/llvm/llvm-project/blob/28981015526f2192440c18f18e8a20cd11b0779c/flang/lib/Semantics/check-omp-structure.cpp#L1446


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110714