[PATCH] D110502: [Flang][openmp] Add semantic checks for OpenMP critical construct name resolution

Thu Oct 7 12:57:44 PDT 2021

kiranchandramohan added a comment.

In D110502#3048310 <https://reviews.llvm.org/D110502#3048310>, @clementval wrote:

> In D110502#3048086 <https://reviews.llvm.org/D110502#3048086>, @NimishMishra wrote:
>
>> In D110502#3034679 <https://reviews.llvm.org/D110502#3034679>, @kiranchandramohan wrote:
>>
>>> Thanks @NimishMishra for taking this forward. Can you include the parser changes also from https://reviews.llvm.org/D93051?
>>
>> Thanks for the comment. I analyzed this as well as worked on a few semantic checks related to hint clause. Are we sure that we require this as a clause list? Because everything is working smoothly anyway. In  D93051 <https://reviews.llvm.org/D93051>, parser changes involves changing the optional clause to a list, of which I do not presently understand the requirement.
>>
>> Also the clause list is no longer a //std::optional//. However, clauses are actually optional. But there exist examples in https://www.openmp.org/wp-content/uploads/openmp-examples-5.0.0.pdf on page 221 which indicates that it is indeed optional.
>
> The clause list should never be optional. Only the clauses inside of the list should be optional or not. D93051 <https://reviews.llvm.org/D93051> is important to make sure we have a homogenous way of dealing with clauses and the OMP.td is correctly defined. Anyway D93051 <https://reviews.llvm.org/D93051> should probably be updated and landed separately if you don't bring these changes in.

+1.

Also, this will reduce the diff between fir-dev and llvm-project/flang since the parser changes are already in fir-dev.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110502