[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 added inline comments.


================
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;
----------------
kiranchandramohan wrote:
> 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?
I have updated to simplify the logic. I am trying to not change parser in first run as there might be some reason why that might have been designed by original authors.
I have no strong opinions to change the parser if this logic still seems harder.
Thanks for `parser::Unwrap` inputs that helped to simplify things @kiranchandramohan.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94087