[PATCH] D106896: [flang][OpenMP] Add parsing support for nontemporal clause.

[Arnamoy B via Phabricator via llvm-commits]

arnamoy10 added inline comments.


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:210
+    "NONTEMPORAL" >> construct<OmpClause>(construct<OmpClause::Nontemporal>(
+                         parenthesized(Parser<OmpNontemporalClause>{}))) ||
     "NOTINBRANCH" >>
----------------
clementval wrote:
> arnamoy10 wrote:
> > clementval wrote:
> > > arnamoy10 wrote:
> > > > clementval wrote:
> > > > > arnamoy10 wrote:
> > > > > > clementval wrote:
> > > > > > > 
> > > > > > Thank you.  With your suggested change, I do not get error while building the compiler, but I get the following error while running `-fopenmp -fsyntax-only` on the examples I provide.  Here is the error:
> > > > > > 
> > > > > > ```
> > > > > >  error: Internal: no symbol found for 'a'
> > > > > >  !$omp target teams distribute simd nontemporal(a)
> > > > > >                                                    ^
> > > > > > ```
> > > > > > 
> > > > > > What am I missing here?
> > > > > This error is coming from the name resolution. So basically your semantic is fine but after the semantic the name resolution fails since `nontemporal` is not in it. You can look at `flang/lib/Semantics/resolve-directives.cpp` and look what is done in the two example below: 
> > > > > 
> > > > > https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/lib/Semantics/resolve-directives.cpp#L363
> > > > > https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/lib/Semantics/resolve-directives.cpp#L405
> > > > Thanks for the pointer.  But as you look in the patch, I am adding a call to `ResolveOmpNameList` in `flang/lib/Semantics/resolve-directives.cpp`, similar to what you indicated.  But why I am still getting error?  
> > > DId you change the signature? It should be in the `bool Pre(const parser::OmpClause::Nontemporal &x)` after the correct change to OMP.td. `OmpNontemporalClause` should not present anymore. 
> > Thanks for the comment.  However, if I change the signature, I am not able to use `ResolveOmpNameList()` similar to a case as per your previous suggestion [[ https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/lib/Semantics/resolve-directives.cpp#L405 | here ]]  How do I do the name resolution then?
> > 
> You probably need to change a bit the code to retrieve the name list. 
> 
> Should probably look like this: https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/lib/Semantics/resolve-directives.cpp#L405
> 
> There are other clauses with name list so basically doing the same as those clauses will work. 
Thanks for the comment.  However all the other clauses that deal with `Namelist` have a struct definition in flang/include/flang/Parser/parse-tree.h, with a memebr in them that holds the list of names (e.g. [[ https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/include/flang/Parser/parse-tree.h#L3373 | here ]], [[ https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/include/flang/Parser/parse-tree.h#L3387 | here ]].  However, as per your suggestions, we do not need a struct definition with update to `Omp.td`.  I am finding this contradictory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106896