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

[sameeran joshi via Phabricator via flang-commits]

sameeranjoshi marked 4 inline comments as done and an inline comment as not done.
sameeranjoshi added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:515
+    context_.Say(clause->source,
+        "Clause ACQUIRE is not allowed on the ATOMIC directive"_err_en_US);
+  }
----------------
clementval wrote:
> sameeranjoshi wrote:
> > clementval wrote:
> > > Why is there a specific check for this? If it is not allowed it should not be in `OMP.td` and then catch be `CheckAllowed` in enter acquire. 
> > Standard says:
> > `If atomic-clause is update or not present then memory-order-clause must not be acq_rel or acquire.`
> > 
> > I couldn't use `CheckNotAllowedIfClause` here reason being there is no `OMPC_atomic`.
> > I see `OMPC_atomic_default_mem_order` but I doubt it's the same what I was trying to find.
> > 
> > also `aquire` `acq_rel` are not allowed only here they might be allowed somewhere else, hence changing them in `OMP.td` would affect other atomic directives as well.
> Then the check for update is missing? and it will fail for atomic read even it should be allowed? 
> Why is there a specific check for this? 
Due to missing clause for `?` inside below function which isn't in `OMP.td`.
```
CheckNotAllowedIfClause( ? ,
      {llvm::omp::Clause::OMPC_acquire, llvm::omp::Clause::OMPC_acq_rel})
```
Reason being that there is nothing mentioned inside TableGen.

> If it is not allowed it should not be in `OMP.td` and then catch be `CheckAllowed` in enter acquire. 

If we remove it from `OMP.td` that would be not found for other atomic clauses where it was expected to work.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:515
+    context_.Say(clause->source,
+        "Clause ACQUIRE is not allowed on the ATOMIC directive"_err_en_US);
+  }
----------------
sameeranjoshi wrote:
> clementval wrote:
> > sameeranjoshi wrote:
> > > clementval wrote:
> > > > Why is there a specific check for this? If it is not allowed it should not be in `OMP.td` and then catch be `CheckAllowed` in enter acquire. 
> > > Standard says:
> > > `If atomic-clause is update or not present then memory-order-clause must not be acq_rel or acquire.`
> > > 
> > > I couldn't use `CheckNotAllowedIfClause` here reason being there is no `OMPC_atomic`.
> > > I see `OMPC_atomic_default_mem_order` but I doubt it's the same what I was trying to find.
> > > 
> > > also `aquire` `acq_rel` are not allowed only here they might be allowed somewhere else, hence changing them in `OMP.td` would affect other atomic directives as well.
> > Then the check for update is missing? and it will fail for atomic read even it should be allowed? 
> > Why is there a specific check for this? 
> Due to missing clause for `?` inside below function which isn't in `OMP.td`.
> ```
> CheckNotAllowedIfClause( ? ,
>       {llvm::omp::Clause::OMPC_acquire, llvm::omp::Clause::OMPC_acq_rel})
> ```
> Reason being that there is nothing mentioned inside TableGen.
> 
> > If it is not allowed it should not be in `OMP.td` and then catch be `CheckAllowed` in enter acquire. 
> 
> If we remove it from `OMP.td` that would be not found for other atomic clauses where it was expected to work.
> 
> Then the check for update is missing?
umm... update check was added and I see that we have 
test with comment `! 2.17.7.6` which works well due to `CheckNotAllowedIfClause` which fits for `llvm::omp::Clause::OMPC_update`

I might not be still clear on how to leverage TableGen for this particular test case.
I am happy to use TableGen wherever possible.
I even tried to add `OMPC_Atomic` but that's not helping.


================
Comment at: flang/lib/Semantics/check-omp-structure.h:155
   void Enter(const parser::OmpClause::IsDevicePtr &);
+  // Memory-order-clause
+  void Enter(const parser::OmpClause::SeqCst &);
----------------
clementval wrote:
> sameeranjoshi wrote:
> > kiranchandramohan wrote:
> > > Thinking about this again. It will be good if we can retain memory-order-clause in the parse tree. Since the standard specifically talks about memory-order-clauses. If we write a frontend tool to list all memory order clauses then if there is a representation for memory-order-clause in the parse tree it will be easy to process all memory-order-clauses.
> > The whole discussion to change parser started when failing to implement semantic checks(given that we had code) without changing parser.
> > 
> > Reason being that `memory-order-clauses` were not part of `OmpClause` inside `openmp-parsers.cpp`.
> > 
> > I am happy to change it If that's possible.
> > Also if @clementval shares some opinions on this issue, he might have a better overview.
> Well, I don't have a strong opinion on that but if you want to use the directive-clauses map generated by TableGen you need to have clauses in OmpClause. 
> 
> I think there could be a way to keep the clauses in OmpClause and have a separate parser that handle the `memory-order-clauses` and that could be applied to Atomic for example. 
Handled by D91839.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89583