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

[Nimish Mishra via Phabricator via llvm-commits]

NimishMishra added a comment.

Thank you for the comments. Please find my views on the same and let me know what you think about this



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:41
+// OpenMPAtomicConstruct
+class OmpAtomicConstructChecker {
+public:
----------------
kiranchandramohan wrote:
> 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;
> };
> ```
It is indeed possible to shift this class to a new file or entirely replace this class by functions instead.

However, the reason to go ahead with this approach was to imitate similar style in //check-omp-structure.cpp//. Two of the classes: //OmpWorkshareBlockChecker// and //OmpCycleChecker// are instantiated only once in the source but still exist as classes rather than a set of independent functions. This was the motivation behind defining a class for my checks as well.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:54
+            [&](const common::Indirection<parser::FunctionReference> &x) {
+              if (!CheckProcedureDesignatorValidity(x)) {
+                context_.Say(expr.source,
----------------
kiranchandramohan wrote:
> 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
Writing like https://github.com/llvm/llvm-project/blob/28981015526f2192440c18f18e8a20cd11b0779c/flang/lib/Semantics/check-omp-structure.cpp#L1446 won't be possible as far as I can see. Reason being instead of working with //parser::DefinedOperator//, we are rather working with operators defined in //parser::Expr// like //parser::Expr::Add// etc. 


Writing like https://github.com/llvm/llvm-project/blob/28981015526f2192440c18f18e8a20cd11b0779c/flang/lib/Semantics/check-omp-structure.cpp#L1415 should be possible. I will rewrite the check accordingly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110714