[flang-commits] [PATCH] D89583: [Flang][OpenMP] Semantic checks for Atomic construct.

[Valentin Clement via Phabricator via flang-commits]

clementval added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:487
+void OmpStructureChecker::Leave(const parser::OmpAtomic &) {
+  if (const auto *clause{FindClause(llvm::omp::Clause::OMPC_acquire)}) {
+    context_.Say(clause->source,
----------------
sameeranjoshi wrote:
> clementval wrote:
> > sameeranjoshi wrote:
> > > clementval wrote:
> > > > Where do you check that it is only for update? 
> > > I check separately for both
> > > `If atomic-clause is update or not present then memory-order-clause must not be acq_rel or acquire.`
> > > break into below
> > > restriction:
> > > ```If atomic-clause is update then memory-order-clause must not be acq_rel or acquire.```
> > > in
> > > `void OmpStructureChecker::Leave(const parser::OmpAtomicUpdate &)`
> > > &
> > > ```If atomic-clause is not present then memory-order-clause must not be acq_rel or acquire.```
> > > in this `Leave`.
> > Ok I see. The `OmpAtomic` is the atomic construct without clause? If so, maybe adding small comment here to precise this would be nice. 
> > Ok I see. The `OmpAtomic` is the atomic construct without clause? If so, maybe adding small comment here to precise this would be nice. 
> 
> To be specific it's not a general clause but called as 'atomic-clause' in standard.
> Which has values read/write/update/capture.
> 
> To answer the question - yes it doesn't have 'atomic-clause'.
> ```
>  !$omp atomic [clause[[,] clause] ... ] <no atomic clause here>
>       update-statement
>  [!$omp end atomic]
> ```
Yeah I get it. Since other "with" clause are represented by a node as well like `OmpAtomicUpdate` I still think it would make sense to have a small comment here to mentioned that the `OmpAtomic` node represent and atomic directive without `atomic-clause`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89583