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

[Nimish Mishra via Phabricator via llvm-commits]

NimishMishra added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:41
+// OpenMPAtomicConstruct
+class OmpAtomicConstructChecker {
+public:
----------------
kiranchandramohan wrote:
> 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.
No I think you raised an important point. I will remove the class and write separate functions instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110714