[flang][OpenMP] Added semantic checks for sections (associated section(s) should be structured block(s)) and simd constructs (associated loop(s) should be structured block(s))

[NimishMishra]

Summary

According to OpenMP 5.0 spec document, the following semantic restrictions have been dealt with in this patch.

1. [sections] Orphaned section directives are prohibited. That is, the section directives must appear within the sections construct and must not be encountered elsewhere in the sections region.
   
   Semantic checks for the following are not necessary, since use of orphaned section construct (i.e. without an enclosing sections directive) throws parser errors and control flow never reaches the semantic checking phase.

Added a test case for the same in llvm-project/flang/test/Parser/omp-sections01.f90

2. [sections] Must be a structured block

Added test case as llvm-project/flang/test/Semantics/omp-sections02.f90 and made changes to branching logic in llvm-project/flang/lib/Semantics/check-directive-structure.h as follows:

• NoBranchingEnforce is used within a parser::Walk called from CheckNoBranching (in check-directive-structure.h itself). CheckNoBranching is invoked from a lot of places, but of our interest in this particular context is void OmpStructureChecker::Enter(const parser::OpenMPSectionsConstruct &x) (defined within llvm-project/flang/lib/Semantics/check-omp-structure.cpp)

• [addition] NoBranchingEnforce lacked tracking context earlier (precisely once control flow entered a OpenMP directive, constructs were no longer being recorded on the ConstructStack). Relevant Pre and Post functions added for the same

• [addition] A EmitBranchOutError on encountering a CALL in a structured block

• [changes] Although NoBranchingEnforce was handling labelled EXITs and CYCLEs well, there were no semantic checks for unlabelled CYCLEs and EXITs. Support added for them as well. For unlabeled cycles and exits, there's a need to efficiently differentiate the ConstructNode added to the stack before the concerned OpenMP directive was encountered and the nodes added after the directive was encountered. The same is done through a simple private unsigned counter within NoBranchingEnforce itself

• [addition] Addition of EmitUnlabelledBranchOutError is due to lack of an error message that gave proper context to the programmer. Existing error messages are either named error messages (which aren't valid for unlabeled cycles and exits) or a simple CYCLE/EXIT statement not allowed within SECTIONS construct which is more generic than the reality.

Other than these implementations, a test case for simd construct has been added.

1. [Test case for simd] Must be a structured block / A program that branches in or out of a function with declare simd is non conforming

Uses the already existing branching out of OpenMP structured block logic as well as changes introduced in point 3 below to semantically validate the restriction. Test case added to llvm-project/flang/test/Semantics/omp-simd01.f90

---

High priority TODOs:

1. (done) Decide whether CALL is an invalid branch out of an OpenMP structured block
2. (done) Fix !$omp do's handling of unlabeled CYCLEs

[NimishMishra]

git-clang-format-7 changes

[NimishMishra]

[NimishMishra]

Fixed !$omp do's handling of unlabeled CYCLEs

[kiranchandramohan]

Thanks @NimishMishra for this patch. I have left some comments.

Could you change the title of the review to explicitly say which semantic check is being implemented?
Can you change the description to first summarize what is being implemented, how it is implemented and then say what is left/TODO?

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

33 ↗

(On Diff #369555)

Can this be along with the other private variables in the bottom of this class?

36 ↗

(On Diff #369555)

I think this can just be an unsigned integer. While we might only require a smaller integer, I don't think we use such smaller integers elsewhere.

82 ↗

(On Diff #369555)

Nit: braced initialization.

flang/test/Parser/omp-sections01.f90

10–15 ↗

(On Diff #369555)

Is the point here that the failure of parsing indicates that orphaned section directives are not permitted?
I think we should have proper error messages instead of a regular parsing failure.

flang/test/Semantics/omp-simd01.f90

1 ↗

(On Diff #369555)

This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.

23 ↗

(On Diff #369555)

Spelling: Invalid

[kiranchandramohan]

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

35 ↗

(On Diff #369555)

Spelling: encountering

56 ↗

(On Diff #369555)

Would using a separate stack be better? Ideally, we should not modify the context stack. If in case this is better then there should be asserts to ensure that the stack is left in the same situation at the end.

66 ↗

(On Diff #369555)

CALL is OK in a structured block.
Remove commented code.

84–85 ↗

(On Diff #369555)

Nit: This comment is not required I think.

87 ↗

(On Diff #369555)

Why is the != check required? When is it equal to x ("DO")?
The comment below is not required.

94–96 ↗

(On Diff #369555)

These comments are also not required.

115 ↗

(On Diff #369555)

Would it be possible to make this error similar to the error in EmitBranchOutErrorWithName?

flang/test/Parser/omp-sections01.f90

1 ↗

(On Diff #369555)

This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.

flang/test/Semantics/omp-sections02.f90

1 ↗

(On Diff #369555)

This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.

[NimishMishra]

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

56 ↗

(On Diff #369555)

I did not use the private stack because since we are already receiving the context as a SemanticsContext& through the class constructor, I thought to use to services of stack push and pop wrapper functions that class SemanticsContext offers.

Check added to ensure the context stack is in the same situation at the end as it was at the beginning.

82 ↗

(On Diff #369555)

Replaced this with a better switch-case construct

87 ↗

(On Diff #369555)

For some directives (for example omp simd or omp do), there has to be a DO following the directive. The parser makes sure of that.

So it doesn't make sense to check unlabeled cycles for such special directives because the parser makes sure we have a matched DO that came after the directive was encountered in the source.

However, the string comparison was sloppy. I was replaced it with a finer switch-case with scope to add more such directives in the future.

flang/test/Parser/omp-sections01.f90

10–15 ↗

(On Diff #369555)

I had a talk with @kiranktp and he opined that the Parser is woefully sloppy with the context it gives to the error messages as well as is in itself complex to change those messages in the first place.

We discussed to return to this Parser's contextual error message issue sometime later. So for now, it's marked as a TODO and the test-case itself is XFAIL

[peixin]

I think your implementation does deal with the first point in your summary. You only add one test case, right?

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

157 ↗

(On Diff #370420)

Instead of giving these comments. Could you please give one comment such as CheckConstructNameBranching(const char *stmt, const parser::Name &stmtName) to differentiate their usage? Maybe this advice is not good.

flang/test/Semantics/omp-sections02.f90

22 ↗

(On Diff #370420)

Add space before err=30. Also please format your test cases. I think fortran test cases use two spaces on the next line instead of four spaces like the style in your part of code in omp-simd01.f90.

flang/test/Semantics/omp-simd01.f90

14 ↗

(On Diff #370420)

Does your implementation support the following code:

!$omp simd
do i = 1, 10
  30 stop
  do j = 1, 10
    goto 30
  end do
end do
!$omp end simd.

If no collapse clause specified, it behaves as if only one loop immediately following is associated with the construct. Could you please add the test in your test case?

[NimishMishra]

In D108904#2981617, @peixin wrote:

I think your implementation does deal with the first point in your summary. You only add one test case, right?

According to the spec document, I had to deal with orphaned section (without an enclosing sections). I added a test case for the same. However, presently, the parser itself is dealing correctly with the issue by giving errors with very less contextual information. Somewhere down the line, we may need to shift this check from the Parser to Semantics, or improve error reporting in the Parser itself. For now, the test case is a XFAIL with a TODO added to it.

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

157 ↗

(On Diff #370420)

I added the comments for easier review and understanding of the changes. Now since there is no problem with the approach taken, I too think I should reduce the verbosity.

flang/test/Semantics/omp-sections02.f90

22 ↗

(On Diff #370420)

I will do this. When the discussion in other comments is resolved, will submit another diff with formatted test cases

flang/test/Semantics/omp-simd01.f90

14 ↗

(On Diff #370420)

The present state of the implementation gives a 'STOP statement is not allowed in a SIMD construct'. Is that what you were looking for?

If not, this was added as void Post(const parser::StopStmt &) { EmitBranchOutError("STOP"); } in NoBranchingEnforce (source: https://reviews.llvm.org/D88655). Possibly @kiranchandramohan can offer some insights. According to his comment there, parallel region can have a STOP. I am not sure about SIMD.

That said, if I replace that STOP statement with something else (like the example below), no errors are generated. That is expected I believe since branching within SIMD's associated DO loop shouldn't be considered a violation of the structured block restriction.

program sample                                                                                                                                                 use omp_lib                                                                                                                                                !$omp simd                                                                                                                                                   do i = 1, 10                                                                                                                                                 30 print*, "sample"                                                                                                                                          do j = 1, 10                                                                                                                                                   goto 30                                                                                                                                                      end do                                                                                                                                                     end do                                                                                                                                                     !$omp end simd                                                                                                                             end program sample

[peixin]

According to the spec document, I had to deal with orphaned section (without an enclosing sections). I added a test case for the same. However, presently, the parser itself is dealing correctly with the issue by giving errors with very less contextual information. Somewhere down the line, we may need to shift this check from the Parser to Semantics, or improve error reporting in the Parser itself. For now, the test case is a XFAIL with a TODO added to it.

I understand this. What I mean is your implementation is not consistent with the summary, which should be fixed since this patch has not implement the semantic check with informative error message. Otherwise, the summary, which should be the commit message, is confusing.

flang/test/Semantics/omp-simd01.f90

14 ↗

(On Diff #370420)

You can replace it with print *, "sample". My key point is to check branching out to the nested loop which is not associated with simd construct. I thought out one more better test case as follows:

!$omp simd collapse(2)
do i = 1, 10
  30 print *, "This is wrong"
  do j = 1, 10
    40 print *, "This is OK"
    do k = 1, 10
      if (expression) then
        goto 30
      end if
      if (expression2) then
        goto 40
      end if
    end do
  end do
end do
!$omp end simd

If my understanding is correct, please polish this test case and add it.

[NimishMishra]

flang/test/Semantics/omp-simd01.f90

14 ↗

(On Diff #370420)

The constraint is there, albeit not because of my changes. When I run this case, it gives a The value of the parameter in the COLLAPSE or ORDERED clause must not be larger than the number of nested loops following the construct.

The reason for this, as it seems to me is, that when we give a COLLAPSE(2), it checks for immediately nesting of exactly 2 DO constructs. Anything else simply raises this error. In this vein, the following modification of this test case succeeds:

!$omp simd collapse(2)
do i = 1, 10  ! Removed 30 print*, ...
  do j = 1, 10
    40 print *, "This is OK"
    do k = 1, 10
      if (expression) then
        goto 30
      end if
      if (expression2) then
        goto 40
      end if
    end do
  end do
end do
!$omp end simd

Similarly, trying COLLAPSE(3) would force removal of the print statement at label 40. So I think we can be reasonably assured that the simd nesting checks implemented in check-omp-structure.cpp will assure absence of any such statements which can violate the structured block restriction.

Please let me know your thoughts on this.

[peixin]

flang/test/Semantics/omp-simd01.f90

14 ↗

(On Diff #370420)

Thanks for the investigation. OK to me now.

[kiranchandramohan]

As requested previously, Could you change the Summary of this PR to

-> What is implemented in this patch.
-> What is not required because it is covered by parsing.
-> What is TODO.
-> Any questions.

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

41 ↗

(On Diff #370420)

Is this copy required if we are using a privateStack? The privateStack will contain the new Constructs. I think the numPrivateConstructs variable is also not required. We can also remove the assert below.

44 ↗

(On Diff #370420)

I don't think we use assert in the Frontend code. It could be DIE that we use. Please check.

The change above might not be required if we don't copy the construct stack.

139 ↗

(On Diff #370420)

Do you need the counter if you are using a private stack?

169 ↗

(On Diff #370420)

If only used here then you can inline the function.

flang/test/Semantics/omp-simd01.f90

14 ↗

(On Diff #370420)

STOP is allowed in OpenMP structured blocks for Fortran.

1 ↗

(On Diff #369555)

Please use test_errors.py. You might need a rebase. OK to delay it till you submit it. Remove the REQUIRES shell line.

[NimishMishra]

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

41 ↗

(On Diff #370420)

Removed everything. Now I have one private vector and handle everything there only. Also removed the counter

44 ↗

(On Diff #370420)

Since I am no longer making a local copy, so this asset as well as the class destructor is no longer there

139 ↗

(On Diff #370420)

No. I have removed that now

169 ↗

(On Diff #370420)

Yes. Inlined the function

flang/test/Semantics/omp-sections02.f90

22 ↗

(On Diff #370420)

Used https://github.com/pseewald/fprettify to format all fortran source.

flang/test/Semantics/omp-simd01.f90

1 ↗

(On Diff #369555)

Shifted from using test_errors.sh. Now the test cases use test_error.py

[kiranchandramohan]

A few more questions and comments.

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

19 ↗

(On Diff #375108)

Nit: Accidentally removed?

121 ↗

(On Diff #375108)

Wouldn't the removal of the reference result in a copy? Is that the intention here?

134 ↗

(On Diff #375108)

CheckForRequiredConstruct -> CheckForRequiredDoConstruct or CheckForDoConstruct

148 ↗

(On Diff #375108)

This function can be a lambda.

160 ↗

(On Diff #375108)

Looking at this again, do we need a construct stack? Can we just maintain a DoConstruct counter (which increments on entering (or Pre) a Do and decrements on leaving (or Post) a Do)? And if the DoConstruct counter value is greater than 0 then we are inside a DoConstruct, else outside.

[NimishMishra]

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

19 ↗

(On Diff #375108)

No I don't think so. I did add #include<assert.h> when I included that assert statement. When that statement was removed, this include was also removed. I think a space may have stayed there, and got removed this time during formatting.

121 ↗

(On Diff #375108)

This is a slip from my end. I did not intend it. Will add the reference back

134 ↗

(On Diff #375108)

Carrying out this discussion in the other comment thread as well: on there being a counter instead of a stack

160 ↗

(On Diff #375108)

Yes it can be. However, note that this class NoBranchingEnforce is used by a parser::Walk within CheckNoBranching function whose aim is to check non-essential branching within OpenMP/OpenACC constructs.

Do you think, in foresight, that this private stack approach would be better than the counter approach? Just in case there may come up a use for some other construct as we expand our OpenMP/OpenACC support.

Similar thought I had for the introduced CheckForRequiredConstruct. As we expand, it could serve as the point of call from various other functions/places aiding them to make a decision on when to throw the illegal branching error.

[kiranchandramohan]

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

160 ↗

(On Diff #375108)

In the past, there have been instances where we have replaced with a more general data-structure. If you can point out a particular case where this will be required then let us keep the construct stack and add a comment saying this will be useful when we implement that case. Otherwise, let us replace it with a counter.

Can you quickly glance through the standard and have a quick check whether there are such checks required? From the top of my head, I don't see anything but I could have missed something.

[NimishMishra]

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

19 ↗

(On Diff #375108)

Please disregard the above comment. This was due to my editor's configuration. I will revert this space back as it was before.

148 ↗

(On Diff #375108)

With the counter approach, I think this won't be necessary anymore.

160 ↗

(On Diff #375108)

I searched through the standard. As far as I can see, I found a few other examples where this could be useful but none of them were without DoConstruct.

So I think I will remove this stack approach now and replace it with a private counter.

[kiranchandramohan]

LGTM. Thanks @NimishMishra for this patch, and addressing all the comments, and for your patience.

I have two Nits. Please have a look, fix if necessary and you can submit. You might have to trim the commit message.

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

148 ↗

(On Diff #376232)

Nit: An underscore at the end to match others?

flang/test/Semantics/omp-simd01.f90

27 ↗

(On Diff #376232)

Nit: Are these ">" format characters. Can you check?

[NimishMishra]

Thanks a lot @kiranchandramohan and @peixin for suggestions on the patch.

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

148 ↗

(On Diff #376232)

Yes will add that.

flang/test/Semantics/omp-simd01.f90

27 ↗

(On Diff #376232)

I used fprettify (https://github.com/pseewald/fprettify) to format the test cases. So yes this is indentation added by the same.

[josh.mottley.arm]

@NimishMishra I believe this patch is causing a false semantic error with SNAP. The error being:

error: Semantic errors in inner.f90
./inner.f90:152:7: error: CYCLE to construct outside of PARALLEL DO construct is not allowed
        IF ( g == 0 ) CYCLE

The code in snap it is erroring at in inner.f90 is:

!$OMP PARALLEL DO NUM_THREADS(nnstd_used) IF(nnstd_used>1)           &
!$OMP& SCHEDULE(STATIC,1) DEFAULT(SHARED) PRIVATE(n,g)               &
!$OMP& PROC_BIND(CLOSE)
  DO n = 1, ng_per_thrd
    g = grp_act(n,t)
    IF ( g == 0 ) CYCLE
    CALL inner_df_calc ( inno, iits(g), flux0pi(:,:,:,g),            &
                      flux0(:,:,:,g), dfmxi(g) )
  END DO
!$OMP END PARALLEL DO

AFAIK this should be valid code, and I know that @Leporacanthicus is also experiencing this issue (although it looks like there could be a secondary issue as well).
I've tried reverting this patch, and managed to stop getting this error when using the flang-omp-report plugin.

[NimishMishra]

In D108904#3096461, @josh.mottley.arm wrote:

@NimishMishra I believe this patch is causing a false semantic error with SNAP. The error being:

error: Semantic errors in inner.f90
./inner.f90:152:7: error: CYCLE to construct outside of PARALLEL DO construct is not allowed
        IF ( g == 0 ) CYCLE

The code in snap it is erroring at in inner.f90 is:

!$OMP PARALLEL DO NUM_THREADS(nnstd_used) IF(nnstd_used>1)           &
!$OMP& SCHEDULE(STATIC,1) DEFAULT(SHARED) PRIVATE(n,g)               &
!$OMP& PROC_BIND(CLOSE)
  DO n = 1, ng_per_thrd
    g = grp_act(n,t)
    IF ( g == 0 ) CYCLE
    CALL inner_df_calc ( inno, iits(g), flux0pi(:,:,:,g),            &
                      flux0(:,:,:,g), dfmxi(g) )
  END DO
!$OMP END PARALLEL DO

AFAIK this should be valid code, and I know that @Leporacanthicus is also experiencing this issue (although it looks like there could be a secondary issue as well).
I've tried reverting this patch, and managed to stop getting this error when using the flang-omp-report plugin.

@josh.mottley.arm Thanks for reporting. I triaged this. It turns out the particular class that is enforcing the semantic checks assumes the OpenMP constructs to be passed to it. In case of looping constructs however, a parser::Walk is called within OpenMP constructs rather than on them. I have provided a fix in https://github.com/llvm/llvm-project/commit/9faed889cfebf5d77faf1fab1ef8f0a2f0255e5c. Please see if this resolves your problems. I also edited a relevant test file to reflect this CYCLE condition. It wasn't there before.

[josh.mottley.arm]

@NimishMishra Thanks for the help. Looks like the patch fixes the issue I was having on my end.

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) {

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