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

[Valentin Clement via Phabricator via flang-commits]

clementval 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)};
----------------
kiranchandramohan wrote:
> sameeranjoshi wrote:
> > 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
> Isn't FindClause used when you are inside a clause and hence do not have access to the construct? If you do it in Enter or Leave of the construct you have access to all the clauses. You might be able to use parser::Unwrap and check whether a clause is present.
`FindClause` might be faster since it's a simple query on a map already populated when the clause is check if it is allowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94087