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

[NimishMishra]

Summary

Taken forward from https://reviews.llvm.org/D93051 (authored by @sameeranjoshi )

As reported in https://bugs.llvm.org/show_bug.cgi?id=48145, name resolution for omp critical construct was failing. This patch adds functionality to help that name resolution as well as implementation to catch name mismatches.

• Changes to check-omp-structure.cpp

In llvm-project/flang/lib/Semantics/check-omp-structure.cpp, under void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x), logic is added to handle the different forms of name mismatches and report appropriate error. The following semantic restrictions are therefore handled here:

1. If a name is specified on a critical directive, the same name must also be specified on the end critical directive
2. If no name appears on the critical directive, no name can appear on the end critical directive
3. If a name appears on either the start critical directive or the end critical directive

The only allowed combinations are: (1) both start and end directives have same name, (2) both start and end directives have NO name altogether

1. A hint clause may not exist on a critical directive that is unnamed

• Changes to resolve-directives.cpp

In llvm-project/flang/lib/Semantics/resolve-directives.cpp, two Pre functions are added for OmpCriticalDirective and OmpEndCriticalDirective that invoke the services of ResolveOmpName to give new symbols to the names. This aids in removing the undesirable behavior noted in https://bugs.llvm.org/show_bug.cgi?id=48145

[clementval]

flang/lib/Semantics/check-omp-structure.cpp

13

unrelated change?

flang/test/Semantics/omp-sync-critical02.f90

17

I'm not sure I follow what this test adds to the same code in omp-sync-critical01.f90...

[NimishMishra]

flang/lib/Semantics/check-omp-structure.cpp

13

Not sure what's caused this. Could it be possible that git-clang-format introduced these?

flang/test/Semantics/omp-sync-critical02.f90

17

I added this verbatim from the artifact at https://bugs.llvm.org/show_bug.cgi?id=48145. Should I remove it?

[clementval]

flang/lib/Semantics/check-omp-structure.cpp

13

Quite unlikely but who knows :-) If you can just undo the change it would be great.

flang/test/Semantics/omp-sync-critical02.f90

17

Looks like it is tested in your omp-sync-critical01.f90 so if it's a duplicate just remove it.

[NimishMishra]

flang/lib/Semantics/check-omp-structure.cpp

13

Removed this

[kiranchandramohan]

Thanks @NimishMishra for taking this forward. Can you include the parser changes also from https://reviews.llvm.org/D93051?

[NimishMishra]

In 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, 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.

[clementval]

In D110502#3048086, @NimishMishra wrote:

In 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, 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 is important to make sure we have a homogenous way of dealing with clauses and the OMP.td is correctly defined. Anyway D93051 should probably be updated and landed separately if you don't bring these changes in.

[kiranchandramohan]

In D110502#3048310, @clementval wrote:

In D110502#3048086, @NimishMishra wrote:

In 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, 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 is important to make sure we have a homogenous way of dealing with clauses and the OMP.td is correctly defined. Anyway 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.

[NimishMishra]

In D110502#3048310, @clementval wrote:

In D110502#3048086, @NimishMishra wrote:

In 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, 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 is important to make sure we have a homogenous way of dealing with clauses and the OMP.td is correctly defined. Anyway D93051 should probably be updated and landed separately if you don't bring these changes in.

Thanks. I understand the necessity of those changes better now. I will bring forward those parser changes in this patch only.

[kiranchandramohan]

@NimishMishra Can you prioritise this patch for completion today?

[NimishMishra]

In D110502#3057634, @kiranchandramohan wrote:

@NimishMishra Can you prioritise this patch for completion today?

Okay. I have added one hint clause semantic check as well. Hope that is fine too. It also has to done with name resolution problem only: namely, a hint clause can't exist on a critical directive that is unnamed.

Everything is ready. As soon as advise on this, I push the revised diff here.

[kiranchandramohan]

In D110502#3057696, @NimishMishra wrote:

In D110502#3057634, @kiranchandramohan wrote:

@NimishMishra Can you prioritise this patch for completion today?

Okay. I have added one hint clause semantic check as well. Hope that is fine too. It also has to done with name resolution problem only: namely, a hint clause can't exist on a critical directive that is unnamed.

Everything is ready. As soon as advise on this, I push the revised diff here.

Yes, please add that check also to this patch.

[kiranchandramohan]

I have one request for change.

I think the other hint clause specific checks can come in a separate patch.

flang/test/Semantics/omp-sync-critical02.f90

47

The standard says the following and I think this is not an error.

"Unless the effect is as if hint(omp_sync_hint_none) was specified, the critical construct must specify a name."

[NimishMishra]

In D110502#3058038, @kiranchandramohan wrote:

I have one request for change.

I think the other hint clause specific checks can come in a separate patch.

Should I revert the hint clause change from this patch?

Yes I am working on a separate patch for hint clauses. Should I move this patch's hint clause test to that one?

flang/test/Semantics/omp-sync-critical02.f90

47

You are right. I missed it

[kiranchandramohan]

In D110502#3058058, @NimishMishra wrote:

In D110502#3058038, @kiranchandramohan wrote:

I have one request for change.

I think the other hint clause specific checks can come in a separate patch.

Should I revert the hint clause change from this patch?

Yes I am working on a separate patch for hint clauses. Should I move this patch's hint clause test to that one?

You can keep this hint check in this patch.

[kiranchandramohan]

LGTM. I have two NIT comments. Please apply these if possible.

Once you submit, can you cherry-pick the changes to the fir-dev branch?

flang/lib/Semantics/check-omp-structure.cpp

1054

Nit: It would be better if you can get the value of the expression and check for it to be ZERO. You can also add a comment above indication what this check is.

| | | | OmpClauseList -> OmpClause -> Hint -> Constant -> Expr = '0_4'
| | | | | Designator -> DataRef -> Name = 'omp_sync_hint_none'

1057

Nit: Consider changing to something like the following.

Hint clause with a value other than omp_sync_hint_none cannot be specified for an unnamed CRITICAL directive.

[NimishMishra]

Thanks @kiranchandramohan and @clementval for comments on the patch. I am working on another hint clause related patch and am taking forward a little TODO to it: improving hint clause comparison.

@kiranchandramohan You mentioned cherry picking this to fir-dev. Do you want just this patch or sections patch as well in the cherry picking? Also whenever your bandwidth allows, please have a quick look at https://reviews.llvm.org/D110714. There's a little question I have posted regarded design choice of a function. Once resolved, I can spend time on that revision too and take it to closure.

Many thanks.

flang/lib/Semantics/check-omp-structure.cpp

1054

Yes this is better.

I gave time to this, but as of now, can't figure our a clean solution to this. The problem is OmpClause defines a variant and that warrants its own std::visit or a HasMember thing.

This hint clause thing is still in my radar though, because a few checks still need to be done for these. So I plan to figure out a clean way (or if I can't, have another function with std::visit to do the comparison) and
come back to change this.

1057

Done

[kiranchandramohan]

In D110502#3058967, @NimishMishra wrote:

Thanks @kiranchandramohan and @clementval for comments on the patch. I am working on another hint clause related patch and am taking forward a little TODO to it: improving hint clause comparison.

OK

@kiranchandramohan You mentioned cherry picking this to fir-dev. Do you want just this patch or sections patch as well in the cherry picking?

Just this patch.

Also whenever your bandwidth allows, please have a quick look at https://reviews.llvm.org/D110714. There's a little question I have posted regarded design choice of a function. Once resolved, I can spend time on that revision too and take it to closure.

I will have a look tomorrow.

flang/lib/Semantics/check-omp-structure.cpp

 //===-- lib/Semantics/check-omp-structure.cpp -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "check-omp-structure.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/tools.h"
 #include <algorithm>
-
 namespace Fortran::semantics {
 
 // Use when clause falls under 'struct OmpClause' in 'parse-tree.h'.
 #define CHECK_SIMPLE_CLAUSE(X, Y) \
   void OmpStructureChecker::Enter(const parser::OmpClause::X &) { \
     CheckAllowed(llvm::omp::Clause::Y); \
   }
 

 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPCancelConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x) {
   const auto &dir{std::get<parser::OmpCriticalDirective>(x.t)};
+  const auto &endDir{std::get<parser::OmpEndCriticalDirective>(x.t)};
   PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_critical);
   const auto &block{std::get<parser::Block>(x.t)};
   CheckNoBranching(block, llvm::omp::Directive::OMPD_critical, dir.source);
+  const auto &dirName{std::get<std::optional<parser::Name>>(dir.t)};
+  const auto &endDirName{std::get<std::optional<parser::Name>>(endDir.t)};
+  if (dirName && endDirName &&
+      dirName->ToString().compare(endDirName->ToString())) {
+    context_
+        .Say(endDirName->source,
+            parser::MessageFormattedText{
+                "CRITICAL directive names do not match"_err_en_US})
+        .Attach(dirName->source, "should be "_en_US);
+  } else if (dirName && !endDirName) {
+    context_
+        .Say(dirName->source,
+            parser::MessageFormattedText{
+                "CRITICAL directive names do not match"_err_en_US})
+        .Attach(dirName->source, "should be NULL"_en_US);
+  } else if (!dirName && endDirName) {
+    context_
+        .Say(endDirName->source,
+            parser::MessageFormattedText{
+                "CRITICAL directive names do not match"_err_en_US})
+        .Attach(endDirName->source, "should be NULL"_en_US);
+  }
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPCriticalConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(
     const parser::OpenMPCancellationPointConstruct &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   const auto &type{std::get<parser::OmpCancelType>(x.t)};
   PushContextAndClauseSets(
       dir.source, llvm::omp::Directive::OMPD_cancellation_point);
   CheckCancellationNest(dir.source, type.v);
 }

flang/lib/Semantics/resolve-directives.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "resolve-directives.h"
-
 #include "check-acc-structure.h"
 #include "check-omp-structure.h"
 #include "resolve-names-utils.h"
 #include "flang/Common/idioms.h"
 #include "flang/Evaluate/fold.h"
 #include "flang/Evaluate/type.h"
 #include "flang/Parser/parse-tree-visitor.h"
 #include "flang/Parser/parse-tree.h"
if (const auto *expr{GetExpr(parsedExpr)}) {
         }
       }
     }
     return true;
   }
 
   bool Pre(const parser::OpenMPBlockConstruct &);
   void Post(const parser::OpenMPBlockConstruct &);
+  bool Pre(const parser::OmpCriticalDirective &x);
+  bool Pre(const parser::OmpEndCriticalDirective &x);
 
   void Post(const parser::OmpBeginBlockDirective &) {
     GetContext().withinConstruct = true;
   }
 
   bool Pre(const parser::OpenMPSimpleStandaloneConstruct &);
   void Post(const parser::OpenMPSimpleStandaloneConstruct &) { PopContext(); }
 
private:
 
   static constexpr Symbol::Flags privateDataSharingAttributeFlags{
       Symbol::Flag::OmpPrivate, Symbol::Flag::OmpFirstPrivate,
       Symbol::Flag::OmpLastPrivate};
 
   static constexpr Symbol::Flags ompFlagsRequireNewSymbol{
       Symbol::Flag::OmpPrivate, Symbol::Flag::OmpLinear,
       Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate,
-      Symbol::Flag::OmpReduction};
+      Symbol::Flag::OmpReduction, Symbol::Flag::OmpCriticalLock};
 
   static constexpr Symbol::Flags ompFlagsRequireMark{
       Symbol::Flag::OmpThreadprivate};
 
   static constexpr Symbol::Flags dataCopyingAttributeFlags{
       Symbol::Flag::OmpCopyIn, Symbol::Flag::OmpCopyPrivate};
 
   std::vector<const parser::Name *> allocateNames_; // on one directive
case llvm::omp::Directive::OMPD_sections:
     break;
   default:
     break;
   }
   ClearDataSharingAttributeObjects();
   return true;
 }
 
+bool OmpAttributeVisitor::Pre(const parser::OmpCriticalDirective &x) {
+  const auto &name{std::get<std::optional<parser::Name>>(x.t)};
+  if (name) {
+    ResolveOmpName(*name, Symbol::Flag::OmpCriticalLock);
+  }
+  return true;
+}
+
+bool OmpAttributeVisitor::Pre(const parser::OmpEndCriticalDirective &x) {
+  const auto &name{std::get<std::optional<parser::Name>>(x.t)};
+  if (name) {
+    ResolveOmpName(*name, Symbol::Flag::OmpCriticalLock);
+  }
+  return true;
+}
+
 bool OmpAttributeVisitor::Pre(const parser::OpenMPCriticalConstruct &x) {
   const auto &criticalDir{std::get<parser::OmpCriticalDirective>(x.t)};
   PushContext(criticalDir.source, llvm::omp::Directive::OMPD_critical);
   return true;
 }
 
 bool OmpAttributeVisitor::Pre(const parser::OpenMPThreadprivate &x) {
   PushContext(x.source, llvm::omp::Directive::OMPD_threadprivate);

 void OmpAttributeVisitor::ResolveOmpName(
     const parser::Name &name, Symbol::Flag ompFlag) {
   if (ResolveName(&name)) {
     if (auto *resolvedSymbol{ResolveOmp(name, ompFlag, currScope())}) {
       if (dataSharingAttributeFlags.test(ompFlag)) {
         AddToContextObjectWithDSA(*resolvedSymbol, ompFlag);
       }
     }
+  } else if (ompFlagsRequireNewSymbol.test(ompFlag)) {
+    const auto pair{GetContext().scope.try_emplace(
+        name.source, Attrs{}, ObjectEntityDetails{})};
+    CHECK(pair.second);
+    name.symbol = &pair.first->second.get();
+  } else {
+    DIE("OpenMP Name resolution failed");
   }
 }
 
 void OmpAttributeVisitor::ResolveOmpNameList(
     const std::list<parser::Name> &nameList, Symbol::Flag ompFlag) {
   for (const auto &name : nameList) {
     ResolveOmpName(name, ompFlag);
   }

flang/test/Semantics/omp-sync-critical01.f90

+! RUN: %python %S/test_errors.py %s %flang -fopenmp
+
+! OpenMP Version 5.0
+! 2.17.1 critical construct
+! CRITICAL start and end CRITICAL directive names mismatch
+integer function timer_tick_sec()
+   implicit none
+   integer t
+
+   !$OMP CRITICAL
+   t = t + 1
+   !$OMP END CRITICAL
+
+   !$OMP CRITICAL (foo)
+   t = t + 1
+   !$OMP END CRITICAL (foo)
+
+   !$OMP CRITICAL (foo)
+   t = t + 1
+   !ERROR: CRITICAL directive names do not match
+   !$OMP END CRITICAL (bar)
+
+   !$OMP CRITICAL (bar)
+   t = t + 1
+   !ERROR: CRITICAL directive names do not match
+   !$OMP END CRITICAL (foo)
+
+   !ERROR: CRITICAL directive names do not match
+   !$OMP CRITICAL (bar)
+   t = t + 1
+   !$OMP END CRITICAL
+
+   !$OMP CRITICAL
+   t = t + 1
+   !ERROR: CRITICAL directive names do not match
+   !$OMP END CRITICAL (foo)
+
+   timer_tick_sec = t
+   return
+
+end function timer_tick_sec

flang/test/Semantics/omp-sync-critical02.f90

+! RUN: %python %S/test_errors.py %s %flang -fopenmp
+
+! OpenMP Version 5.0
+! 2.17.1 critical construct
+! Test case added to ensure proper semantic checks of CRITICAL names. See https://bugs.llvm.org/show_bug.cgi?id=48145 for details
+integer function timer_tick_sec()
+   implicit none
+   integer t
+
+   !$OMP CRITICAL (foo)
+   t = t + 1
+   !$OMP END CRITICAL (foo)
+
+   timer_tick_sec = t
+   return
+
+end function timer_tick_sec