[Flang][openmp] Add semantic checks for OpenMP critical construct.

[sameeranjoshi]

Summary

Name resolution was probably failing in bug reported at
https://bugs.llvm.org/show_bug.cgi?id=48145
as the omp critical construct needs a new symbol for the name which was
missing. This is fixed by creating a new symbol during name resolution
for a new flag OmpCriticalLock.

Upgrade to OMP-5.0 standard.

Checks for restrictions from OMP-5.0 are added.

• If a name is specified on a critical directive, the same name must also be specified on the

end critical directive.

• If no name appears on the critical directive, no name can appear on the end critical

directive.

• Unless the effect is as if hint(omp_sync_hint_none) was specified, the critical

construct must specify a name.

A new routine CheckNameMatching mostly follows CheckEndName from resolve-labels.cpp.

[clementval]

flang/lib/Semantics/resolve-directives.cpp

368–369

What's the big difference with ResolveOmpName. Merging the two functions would be better.

[sameeranjoshi]

flang/lib/Semantics/resolve-directives.cpp

368–369

That's a good catch.
If you see ResolveName was implemented before this patch.
This patch highlighted the need to extend the name resolution logic, which was just extended inside ResolveName and no other code was affected(except that we need a one time signature change with this patch).
As discussed, there is no way I could find to reuse code from resolve-names.cpp inside this file, hence we had a short/quick way to implement ResolveName inside this file.
Being said that I still see with some new construct the ResolveName inside this file might need some change.
Hence I extracted into another function.
Which makes only changes to the above function and not much changes in other code.

If there's still something you see of no use to have separate functions I can refactor them and collapse them into 1.
Thank you.

[clementval]

flang/lib/Semantics/resolve-directives.cpp

368–369

Would be better to have a single function.

[sameeranjoshi]

Merge ResolveName into ResolveOmpName.

[tskeith]

flang/lib/Semantics/resolve-directives.cpp

1065

What is the purpose of introducing maybeName when you can just use name?

[sameeranjoshi]

Remove unnecessary variable mayBeName.

[sameeranjoshi]

flang/lib/Semantics/resolve-directives.cpp

1065

Thanks.
Not sure what I was trying to do here.

[clementval]

flang/lib/Semantics/resolve-directives.cpp

1000

Does it really make sense to push two contexts? If yes, you might want to have a visitor function for parser::OmpCriticalDirective and parser::OmpEndCriticalDirective where you push their respective context.

1074

Do you want the program to abort or would it be better to have a return value to this function and generate appropriate user facing error message?

1076

Are those tabs?

[sameeranjoshi]

flang/lib/Semantics/resolve-directives.cpp

1000

No it doesn't will remove the redundant one.

1074

I think aborting it seems better.
Consider a test case which uses Symbol::Flags other than ompFlagsRequireNewSymbol.
for e.g Using shared clause and there is a missing declaration for symbol foo.

integer function timer_tick_sec()
  implicit none
  integer t

  !$OMP parallel shared(foo)
  t = t + 1
  !$OMP END parallel

  timer_tick_sec = t
  return

end function timer_tick_sec

Above test case gives the user error message as
No explicit type declared for 'foo' which seems the correct error for above test case as it's pointing the errors in users code.

DIE seems to be an error internal to the compiler and there could be a problem with the ResolveOmpName and the user might not be responsible for that.

If you have a test which you think might fail, could you please point it?

1076

no.

[kiranchandramohan]

flang/lib/Semantics/check-directive-structure.h

278–282

Can you use parser::Unwrap<parser::Name> to simplify the code?

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

243

The module file has omp_sync_hint_none.
https://github.com/llvm/llvm-project/blob/3729ee893948be7e3ba138b2a04c4cdfa6257cdf/flang/module/omp_lib.h#L46

260

Can this be part of Enter? Or is there a convention that checking clauses will only happen during leave?

flang/lib/Semantics/resolve-directives.cpp

334

Does lock need a new symbol? What is the issue you saw?

1003

Can the name here be a variable declared elsewhere? If not do we have to resolve the name?

flang/include/flang/Parser/parse-tree.h

struct OpenMPDeclarativeConstruct {
   UNION_CLASS_BOILERPLATE(OpenMPDeclarativeConstruct);
   CharBlock source;
   std::variant<OpenMPDeclarativeAllocate, OpenMPDeclareReductionConstruct,
       OpenMPDeclareSimdConstruct, OpenMPDeclareTargetConstruct,
       OpenMPThreadprivate>
       u;
 };
 
-// 2.13.2 CRITICAL [Name] <block> END CRITICAL [Name]
 struct OmpCriticalDirective {
   TUPLE_CLASS_BOILERPLATE(OmpCriticalDirective);
   CharBlock source;
-  std::tuple<Verbatim, std::optional<Name>, std::optional<OmpClause>> t;
+  std::tuple<Verbatim, std::optional<Name>, OmpClauseList> t;
 };
+
 struct OmpEndCriticalDirective {
   TUPLE_CLASS_BOILERPLATE(OmpEndCriticalDirective);
   CharBlock source;
   std::tuple<Verbatim, std::optional<Name>> t;
 };
+
+// [OMP-5.0] 2.17.1 CRITICAL [(Name) [[,] hint(hint-expression)]]
+//                  <block>
+//                  END CRITICAL [Name]
 struct OpenMPCriticalConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPCriticalConstruct);
   std::tuple<OmpCriticalDirective, Block, OmpEndCriticalDirective> t;
 };
 
 // 2.11.3 allocate -> ALLOCATE [(variable-name-list)] [clause]
 //        [ALLOCATE (variable-name-list) [clause] [...]]
 //        allocate-statement

flang/lib/Parser/openmp-parsers.cpp

TYPE_PARSER(construct<OpenMPAtomicConstruct>(Parser<OmpAtomicRead>{}) ||
     construct<OpenMPAtomicConstruct>(Parser<OmpAtomic>{}))
 
 // 2.13.2 OMP CRITICAL
 TYPE_PARSER(startOmpLine >>
     sourced(construct<OmpEndCriticalDirective>(
         verbatim("END CRITICAL"_tok), maybe(parenthesized(name)))) /
         endOmpLine)
 TYPE_PARSER(sourced(construct<OmpCriticalDirective>(verbatim("CRITICAL"_tok),
-                maybe(parenthesized(name)), maybe(Parser<OmpClause>{}))) /
+                maybe(parenthesized(name)), Parser<OmpClauseList>{})) /
     endOmpLine)
 
 TYPE_PARSER(construct<OpenMPCriticalConstruct>(
     Parser<OmpCriticalDirective>{}, block, Parser<OmpEndCriticalDirective>{}))
 
 // 2.11.3 Executable Allocate directive
 TYPE_PARSER(
     sourced(construct<OpenMPExecutableAllocate>(verbatim("ALLOCATE"_tok),

flang/lib/Parser/unparse.cpp

void Unparse(const OpenMPDeclarativeAllocate &x) {
     Walk(std::get<OmpClauseList>(x.t));
     Put("\n");
     EndOpenMP();
   }
   void Unparse(const OmpCriticalDirective &x) {
     BeginOpenMP();
     Word("!$OMP CRITICAL");
     Walk(" (", std::get<std::optional<Name>>(x.t), ")");
-    Walk(std::get<std::optional<OmpClause>>(x.t));
+    Walk(std::get<OmpClauseList>(x.t));
     Put("\n");
     EndOpenMP();
   }
   void Unparse(const OmpEndCriticalDirective &x) {
     BeginOpenMP();
     Word("!$OMP END CRITICAL");
     Walk(" (", std::get<std::optional<Name>>(x.t), ")");
     Put("\n");

flang/lib/Semantics/check-directive-structure.h

 // directive language.
 
 #ifndef FORTRAN_SEMANTICS_CHECK_DIRECTIVE_STRUCTURE_H_
 #define FORTRAN_SEMANTICS_CHECK_DIRECTIVE_STRUCTURE_H_
 
 #include "flang/Common/enum-set.h"
 #include "flang/Semantics/semantics.h"
 #include "flang/Semantics/tools.h"
-
 #include <unordered_map>
 
 namespace Fortran::semantics {
 
 template <typename C, std::size_t ClauseEnumSize> struct DirectiveClauses {
   const common::EnumSet<C, ClauseEnumSize> allowed;
   const common::EnumSet<C, ClauseEnumSize> allowedOnce;
   const common::EnumSet<C, ClauseEnumSize> allowedExclusive;
protected:
 
   template <typename B> void CheckMatching(const B &beginDir, const B &endDir) {
     const auto &begin{beginDir.v};
     const auto &end{endDir.v};
     if (begin != end) {
       SayNotMatching(beginDir.source, endDir.source);
     }
   }
+
+  // Checks for missing or mismatching names on OpenMP/OpenACC directives
+  // of two different types.
+  template <typename T1, typename T2>
+  void CheckNameMatching(const T1 &, const T2 &);
+
   void CheckNoBranching(const parser::Block &block, D directive,
       const parser::CharBlock &directiveSource);
 
   // Check that only clauses in set are after the specific clauses.
   void CheckOnlyAllowedAfter(C clause, common::EnumSet<C, ClauseEnumSize> set);
 
   void CheckRequired(C clause);
 
protected:
   std::vector<DirectiveContext> dirContext_; // used as a stack
   std::unordered_map<D, DirectiveClauses<C, ClauseEnumSize>>
       directiveClausesMap_;
 
   std::string ClauseSetToString(const common::EnumSet<C, ClauseEnumSize> set);
 };
 
 template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
+template <typename T1, typename T2>
+void DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::CheckNameMatching(
+    const T1 &beginDir, const T2 &endDir) {
+  const auto &directiveName{std::get<parser::Verbatim>(beginDir.t)};
+  const auto &upperCaseDirName{
+      parser::ToUpperCaseLetters(directiveName.source.ToString())};
+  const auto &beginDirName{std::get<std::optional<parser::Name>>(beginDir.t)};
+  const auto &endDirName{std::get<std::optional<parser::Name>>(endDir.t)};
+
+  if (beginDirName) {
+    if (endDirName) {
+      // Start and end directive names mismatch.
+      if (beginDirName->source != endDirName->source) {
+        context_
+            .Say(endDirName->source,
+                parser::MessageFormattedText{
+                    "%s directive name mismatch"_err_en_US, upperCaseDirName})
+            .Attach(beginDirName->source, "should be"_en_US);
+      }
+    } else { // Missing end directive name
+      context_
+          .Say(endDir.source,
+              parser::MessageFormattedText{
+                  "%s directive name required but missing"_err_en_US,
+                  upperCaseDirName})
+          .Attach(beginDirName->source, "should be"_en_US);
+    }
+  } else if (endDirName) { // Missing start directive name
+    context_
+        .Say(endDirName->source,
+            parser::MessageFormattedText{
+                "%s directive name unexpected"_err_en_US, upperCaseDirName})
+        .Attach(
+            beginDir.source, "unnamed %s directive"_en_US, upperCaseDirName);
+  }
+}
+
+template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
 void DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::CheckNoBranching(
     const parser::Block &block, D directive,
     const parser::CharBlock &directiveSource) {
   NoBranchingEnforce<D> noBranchingEnforce{
       context_, directiveSource, directive, ContextDirectiveAsFortran()};
   parser::Walk(block, noBranchingEnforce);
 }
 

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

#include "llvm/Frontend/OpenMP/OMP.cpp.inc"
   void Enter(const parser::OmpClause::UseDevicePtr &);
   void Enter(const parser::OmpClause::IsDevicePtr &);
   // Memory-order-clause
   void Enter(const parser::OmpClause::SeqCst &);
   void Enter(const parser::OmpClause::AcqRel &);
   void Enter(const parser::OmpClause::Release &);
   void Enter(const parser::OmpClause::Acquire &);
   void Enter(const parser::OmpClause::Relaxed &);
+  void Enter(const parser::OmpClause::Hint &);
 
   void Enter(const parser::OmpAlignedClause &);
   void Enter(const parser::OmpAllocateClause &);
   void Enter(const parser::OmpDefaultClause &);
   void Enter(const parser::OmpDefaultmapClause &);
   void Enter(const parser::OmpDependClause &);
   void Enter(const parser::OmpDistScheduleClause &);
   void Enter(const parser::OmpIfClause &);

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

void OmpStructureChecker::Enter(const parser::OpenMPCancelConstruct &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_cancel);
 }
 
 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)};
-  PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_critical);
+// TODO: Change it to a more better version without raw string comparision
+// once the module files are present.
+static bool IsHintExprAs(
+    const parser::Expr &expr, const std::string &expectedString) {
+  return (expr.source.ToString() != expectedString) ? false : true;
 }
 
-void OmpStructureChecker::Leave(const parser::OpenMPCriticalConstruct &) {
+void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x) {
+  const auto &beginCriticalDir{std::get<parser::OmpCriticalDirective>(x.t)};
+  PushContextAndClauseSets(
+      beginCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+  const auto &endCriticalDir{std::get<parser::OmpEndCriticalDirective>(x.t)};
+  PushContextAndClauseSets(
+      endCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+  CheckNameMatching<parser::OmpCriticalDirective,
+      parser::OmpEndCriticalDirective>(beginCriticalDir, endCriticalDir);
+}
+
+void OmpStructureChecker::Leave(const parser::OpenMPCriticalConstruct &x) {
+  // [OMP-5.0][2.17.1] Unless the effect is as if hint(omp_sync_hint_none)
+  // was specified, the critical construct must specify a name
+  if (auto *clause{FindClause(llvm::omp::Clause::OMPC_hint)}) {
+    const auto &hintClause{std::get<parser::OmpClause::Hint>(clause->u)};
+    const parser::Expr &expr{hintClause.v.thing.value()};
+    if (!IsHintExprAs(expr, "omp_sync_hint_none")) {
+      const auto &beginCriticalDir{std::get<parser::OmpCriticalDirective>(x.t)};
+      const auto &criticalName{
+          std::get<std::optional<parser::Name>>(beginCriticalDir.t)};
+      if (!criticalName) {
+        context_.Say(beginCriticalDir.source,
+            "CRITICAL construct must specify a name expect unless the effect"
+            " is equivalent to specifying a HINT(omp_sync_hint_none)"_err_en_US);
+      }
+    }
+  }
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(
     const parser::OpenMPCancellationPointConstruct &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   PushContextAndClauseSets(
       dir.source, llvm::omp::Directive::OMPD_cancellation_point);

 CHECK_SIMPLE_CLAUSE(Uniform, OMPC_uniform)
 CHECK_SIMPLE_CLAUSE(Untied, OMPC_untied)
 CHECK_SIMPLE_CLAUSE(UseDevicePtr, OMPC_use_device_ptr)
 CHECK_SIMPLE_CLAUSE(AcqRel, OMPC_acq_rel)
 CHECK_SIMPLE_CLAUSE(Acquire, OMPC_acquire)
 CHECK_SIMPLE_CLAUSE(SeqCst, OMPC_seq_cst)
 CHECK_SIMPLE_CLAUSE(Release, OMPC_release)
 CHECK_SIMPLE_CLAUSE(Relaxed, OMPC_relaxed)
+CHECK_SIMPLE_CLAUSE(Hint, OMPC_hint)
 
 CHECK_REQ_SCALAR_INT_CLAUSE(Allocator, OMPC_allocator)
 CHECK_REQ_SCALAR_INT_CLAUSE(Grainsize, OMPC_grainsize)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTasks, OMPC_num_tasks)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTeams, OMPC_num_teams)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumThreads, OMPC_num_threads)
 CHECK_REQ_SCALAR_INT_CLAUSE(Priority, OMPC_priority)
 CHECK_REQ_SCALAR_INT_CLAUSE(ThreadLimit, OMPC_thread_limit)

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"
bool Pre(const parser::OpenMPDeclareSimdConstruct &x) {
     PushContext(x.source, llvm::omp::Directive::OMPD_declare_simd);
     const auto &name{std::get<std::optional<parser::Name>>(x.t)};
     if (name) {
       ResolveOmpName(*name, Symbol::Flag::OmpDeclareSimd);
     }
     return true;
   }
   void Post(const parser::OpenMPDeclareSimdConstruct &) { PopContext(); }
+  bool Pre(const parser::OpenMPCriticalConstruct &x);
+  void Post(const parser::OpenMPCriticalConstruct &) { PopContext(); }
   bool Pre(const parser::OpenMPThreadprivate &);
   void Post(const parser::OpenMPThreadprivate &) { PopContext(); }
 
   // 2.15.3 Data-Sharing Attribute Clauses
   void Post(const parser::OmpDefaultClause &);
   bool Pre(const parser::OmpClause::Shared &x) {
     ResolveOmpObjectList(x.v, Symbol::Flag::OmpShared);
     return false;
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};
 
   std::vector<const parser::Name *> allocateNames_; // on one directive
private:
   void ResolveSeqLoopIndexInParallelOrTaskConstruct(const parser::Name &);
 
   void ResolveOmpObjectList(const parser::OmpObjectList &, Symbol::Flag);
   void ResolveOmpObject(const parser::OmpObject &, Symbol::Flag);
   Symbol *ResolveOmp(const parser::Name &, Symbol::Flag, Scope &);
   Symbol *ResolveOmp(Symbol &, Symbol::Flag, Scope &);
   Symbol *ResolveOmpCommonBlockName(const parser::Name *);
   void ResolveOmpNameList(const std::list<parser::Name> &, Symbol::Flag);
   void ResolveOmpName(const parser::Name &, Symbol::Flag);
-  Symbol *ResolveName(const parser::Name *);
   Symbol *DeclareOrMarkOtherAccessEntity(const parser::Name &, Symbol::Flag);
   Symbol *DeclareOrMarkOtherAccessEntity(Symbol &, Symbol::Flag);
   void CheckMultipleAppearances(
       const parser::Name &, const Symbol &, Symbol::Flag);
 
   void CheckDataCopyingClause(
       const parser::Name &, const Symbol &, Symbol::Flag);
 
   void CheckAssocLoopLevel(std::int64_t level, const parser::OmpClause *clause);
case llvm::omp::Directive::OMPD_sections:
     break;
   default:
     break;
   }
   ClearDataSharingAttributeObjects();
   return true;
 }
 
+bool OmpAttributeVisitor::Pre(const parser::OpenMPCriticalConstruct &x) {
+  const auto &beginCriticalDir{std::get<parser::OmpCriticalDirective>(x.t)};
+  const auto &endCriticalDir{std::get<parser::OmpEndCriticalDirective>(x.t)};
+  PushContext(beginCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+  PushContext(endCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+  if (const auto &criticalName{
+          std::get<std::optional<parser::Name>>(beginCriticalDir.t)}) {
+    ResolveOmpName(*criticalName, Symbol::Flag::OmpCriticalLock);
+  }
+  if (const auto &endCriticalName{
+          std::get<std::optional<parser::Name>>(endCriticalDir.t)}) {
+    ResolveOmpName(*endCriticalName, Symbol::Flag::OmpCriticalLock);
+  }
+  return true;
+}
+
 bool OmpAttributeVisitor::Pre(const parser::OpenMPThreadprivate &x) {
   PushContext(x.source, llvm::omp::Directive::OMPD_threadprivate);
   const auto &list{std::get<parser::OmpObjectList>(x.t)};
   ResolveOmpObjectList(list, Symbol::Flag::OmpThreadprivate);
   return true;
 }
 
 void OmpAttributeVisitor::Post(const parser::OmpDefaultClause &x) {
if (!symbol->owner().IsDerivedType() && !symbol->has<ProcEntityDetails>() &&
               "a data-sharing attribute clause"_err_en_US,
               symbol->name());
         }
       }
     }
   } // within OpenMP construct
 }
 
-Symbol *OmpAttributeVisitor::ResolveName(const parser::Name *name) {
-  if (auto *resolvedSymbol{
-          name ? GetContext().scope.FindSymbol(name->source) : nullptr}) {
-    name->symbol = resolvedSymbol;
-    return resolvedSymbol;
+void OmpAttributeVisitor::ResolveOmpName(
+    const parser::Name &name, Symbol::Flag ompFlag) {
+  if (&name) {
+    if (auto *resolvedSymbol{GetContext().scope.FindSymbol(name.source)}) {
+      Resolve(name, resolvedSymbol);
+    } else if (ompFlagsRequireNewSymbol.test(ompFlag)) {
+      // Create a new symbol.
+      const auto pair{GetContext().scope.try_emplace(
+          name.source, Attrs{}, ObjectEntityDetails{})};
+      CHECK(pair.second);
+      name.symbol = &pair.first->second.get();
     } else {
-    return nullptr;
+      DIE("OpenMP Name resolution failed");
     }
   }
 
-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);
-      }
-    }
-  }
-}
 
 void OmpAttributeVisitor::ResolveOmpNameList(
     const std::list<parser::Name> &nameList, Symbol::Flag ompFlag) {
   for (const auto &name : nameList) {
     ResolveOmpName(name, ompFlag);
   }
 }
 

flang/test/Semantics/omp-clause-validity01.f90

! Standalone Directives (basic)
   !ERROR: RELAXED clause is not allowed on the FLUSH directive
   !$omp flush relaxed
 
   !$omp cancel DO
   !$omp cancellation point parallel
 
 ! 2.13.2 critical Construct
 
-  !ERROR: Internal: no symbol found for 'first'
   !$omp critical (first)
   a = 3.14
-  !ERROR: Internal: no symbol found for 'first'
   !$omp end critical (first)
 
 ! 2.9.1 task-clause -> if-clause |
 !                      final-clause |
 !                      untied-clause |
 !                      default-clause |
 !                      mergeable-clause |
 !                      private-clause |

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

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+use omp_lib
+  implicit none
+  integer :: maxival  = -100, i, j
+  integer :: sharedarray(10000) = (/ (j, j=1, 10000) /)
+  !$omp parallel do num_threads(4)
+  do i=1, size(sharedarray), 1
+  !$omp critical
+    ! Performance loss
+    if (sharedarray(i) .gt. maxival) then
+      maxival = sharedarray(i)
+    end if
+  !$omp end critical
+  end do
+  !$omp end parallel do
+  print *, "Max val in array is = ", maxival
+
+  !$omp parallel do num_threads(4)
+  do i=1, size(sharedarray), 2
+  !$omp critical (somename)
+    if (sharedarray(i) .gt. maxival) then
+      maxival = sharedarray(i)
+    end if
+  !ERROR: CRITICAL directive name required but missing
+  !$omp end critical
+  end do
+  !$omp end parallel do
+  print *, "Max val in array is = ", maxival
+
+  !$omp parallel do num_threads(4)
+  do i=1, size(sharedarray), 3
+  !$omp critical
+    if (sharedarray(i) .gt. maxival) then
+      maxival = sharedarray(i)
+    end if
+  !ERROR: CRITICAL directive name unexpected
+  !$omp end critical (somename)
+  end do
+  !$omp end parallel do
+  print *, "Max val in array is = ", maxival
+
+  !$omp parallel do num_threads(4)
+  do i=1, size(sharedarray), 4
+  !$omp critical(somename)
+    if (sharedarray(i) .gt. maxival) then
+      maxival = sharedarray(i)
+    end if
+  !$omp end critical(somename)
+  end do
+  !$omp end parallel do
+  print *, "Max val in array is = ", maxival
+
+  !$omp parallel do num_threads(4)
+  do i=1, size(sharedarray), 5
+  !$omp critical(somename)
+    if (sharedarray(i) .gt. maxival) then
+      maxival = sharedarray(i)
+    end if
+  !ERROR: CRITICAL directive name mismatch
+  !$omp end critical(othername)
+    end do
+  !$omp end parallel do
+  print *, "Max val in array is = ", maxival
+
+  !Unless the effect is as if hint(omp_sync_hint_none) was specified, the
+  !critical construct must specify a name.
+  !$omp critical(foo) hint(omp_sync_hint_none)
+  !$omp end critical(foo)
+
+  !ERROR: CRITICAL construct must specify a name expect unless the effect is equivalent to specifying a HINT(omp_sync_hint_none)
+  !$omp critical hint(omp_sync_hint_uncontended)
+  !$omp end critical
+
+  !$omp critical(foo) hint(omp_sync_hint_uncontended)
+  !$omp end critical(foo)
+
+  !$omp critical(foo)
+  !$omp end critical(foo)
+
+  !$omp critical(foo) hint(omp_sync_hint_none) hint(omp_sync_hint_none)
+  !$omp end critical(foo)
+
+  !ERROR: PRIVATE clause is not allowed on the END CRITICAL(FOO) directive
+  !$omp critical(foo) hint(omp_sync_hint_none) private(i)
+  !$omp end critical(foo)
+
+end program

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

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! see https://bugs.llvm.org/show_bug.cgi?id=48145
+
+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