[PATCH] D110714: [Flang][openmp] Added semantic checks for atomic construct
Kiran Chandramohan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 29 15:07:23 PDT 2021
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
More information about the llvm-commits
mailing list