[flang-commits] [PATCH] D94087: [flang][openmp]At most one threads, simd and depend clause can appear on OpenMP ORDERED construct.

[sameeran joshi via Phabricator via flang-commits]

sameeranjoshi marked an inline comment as done.
sameeranjoshi added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:138
 
-void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &) {
+void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &x) {
+  const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
----------------
clementval wrote:
> clementval wrote:
> > sameeranjoshi wrote:
> > > kiranchandramohan wrote:
> > > > Why is this handled on Leave and not Enter like others?
> > > With the current approach (traversing an `OmpClauseList`) it could have worked inside `Enter`.
> > > But here's are a few reasons not to use 
> > > 
> > > * I prefer doing checks inside `Leave`, so that the checks remain at a single location and `Enter` does the context pushing and other stuff.
> > > * I had initially plan to use `FindClause` as that saves us from writing more code and code looks more readable, unfortunately that did not work in the first run as `ResetPartialContext` clears the members in `DirectiveContext` which has a reason because `OmpBeginBlockDirective` & `OmpEndBlockDirective` have different set of clauses based on the context.
> > > To resolve this will add `Leave` for `OmpBeginBlockDirective`.
> > > ```
> > >   void Enter(const parser::OpenMPBlockConstruct &);
> > >   void Leave(const parser::OpenMPBlockConstruct &);
> > >   void Leave(const parser::OmpBeginBlockDirective &);
> > >   void Enter(const parser::OmpEndBlockDirective &);
> > > ```
> > > * `FindClause` doesn't work inside `Enter` as the list is empty and it's populated only when we visit each clause.
> > >    `grep FindClause` and see it's not found inside any `Enter` function.
> > > 
> > Good point.
> What is blocking you now to use `FindClause`? The `ResetPartialContext` should not prevent you to use `FindClause` since you are checking different sets of clauses. 
I will send a patch for the same.
I am trying to understand one more comment
https://reviews.llvm.org/D94087#inline-889166


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94087