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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:41
+// OpenMPAtomicConstruct
+class OmpAtomicConstructChecker {
+public:
----------------
NimishMishra wrote:
> 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.
I think a workshare construct can contain arbitrary blocks of code and hence it is difficult to extract out an Assignment statement or expression from there. But in the atomic construct case, the region it contains is much simpler and it is easy to extract out the Assignment Statement from the atomic construct itself.

So I would recommend implementing as functions unless you have a strong objection.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:54
+            [&](const common::Indirection<parser::FunctionReference> &x) {
+              if (!CheckProcedureDesignatorValidity(x)) {
+                context_.Say(expr.source,
----------------
NimishMishra wrote:
> 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
OK. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110714