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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan 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)};
----------------
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.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:838-875
+    const parser::OmpDependClause::Source *foundSource{nullptr};
+    const parser::OmpDependClause::Source *foundSourceFromEnd{nullptr};
+    const parser::OmpDependClause::Sink *foundSink{nullptr};
+    for (const auto *vecElem : ompDependVector) {
+      foundSource = std::get_if<parser::OmpDependClause::Source>(&vecElem->u);
+      if (foundSource != nullptr) {
+        break;
----------------
sameeranjoshi wrote:
> kiranchandramohan wrote:
> > ```
> > struct OmpDependenceType {
> >   ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
> >   WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
> > };
> > 
> > struct OmpDependClause {
> > ....
> >   struct InOut {
> >     TUPLE_CLASS_BOILERPLATE(InOut);
> >     std::tuple<OmpDependenceType, std::list<Designator>> t;
> >   };
> >   std::variant<Source, Sink, InOut> u;
> > };
> > ```
> > Can you use OmpDependenceType  in OmpDependClause to simplify this logic? I think you can just count the source, sink and print out the error messages.
> Could anyone please elaborate more on above?
> I couldn't understand .
> `parse-tree.h` mentions
> ```
> // 2.13.9 depend-type -> IN | OUT | INOUT | SOURCE | SINK
> struct OmpDependenceType {
>   ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
>   WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
> };
> ```
> Where as parsers omit's the `source` and sink  in `OmpDependenceType` in `openmp-parsers.cpp`
> 
> ```
> TYPE_PARSER(
>     construct<OmpDependenceType>("IN"_id >> pure(OmpDependenceType::Type::In) ||
>         "INOUT" >> pure(OmpDependenceType::Type::Inout) ||
>         "OUT" >> pure(OmpDependenceType::Type::Out))) 
> ```
> 
> I was confused if the parse tree has some issues.
> Hence, if you somecan elaborate a bit on what exactly is expected that would be really helpful.
> Or if there's a more better way of simplifying this code.
I was assuming that the dependence_type is set always. If it is not then can it be set in the parser and used? Is there any reason why it is not set?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94087