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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:210
+    "NONTEMPORAL" >> construct<OmpClause>(construct<OmpClause::Nontemporal>(
+                         parenthesized(Parser<OmpNontemporalClause>{}))) ||
     "NOTINBRANCH" >>
----------------
arnamoy10 wrote:
> 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.
This is not true. `use_device_ptr` and `is_device_ptr ` are two examples that deal with name list and use the TableGen file the right way. Other clause that have a struct are probably something leftover from when we moved to TableGen and we should correct them as well. There is no need os an extra struct for these clauses and that not how the TableGen file should be used. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106896