[flang][OpenMP] Support privatization for single construct

[peixin]

Summary

This supports the lowering of private and firstprivate clauses in single
construct. The alloca ops are emitted in the entry block according to
https://llvm.org/docs/Frontend/PerformanceTips.html#use-of-allocas, and
the load/store ops are emitted in the single region. The data race
problem is handled in OMPIRBuilder. That is, the barrier is emitted in
OMPIRBuilder.

Co-authored-by: Nimish Mishra <[e-mail]>

[peixin]

I am preparing one patch to fix the same data-race problem in classic-flang. I noticed it seems the barrier is not necessary for task and single constructs. BTW, our test team found that the data race problem is more obvious when both firstprivate and lastprivate exist, even not only for pointer variables. Once we agree on a final decision on the data-race problem, I will submit one similar patch to classic-flang, too.

[domada]

@peixin What is status of this patch? Are you planning to modify this patch or is it ready for the review?

[peixin]

@domada I will update this patch later this week.

[peixin]

Rebase and address the data race problem for single construct.

[domada]

flang/test/Lower/OpenMP/single.f90

2

Why do you use bbc instead of flang-new?

[peixin]

flang/test/Lower/OpenMP/single.f90

2

I use both bbc and flang-new for lowering test. I also remove the fir-opt check since it should not be here.

@kiranchandramohan planned to change the lowering tests (removing the lowering to LLVMIR tests under flang/test/Lower/OpenMP) after upstreaming is done. @kiranchandramohan When do you plan to change them?

[domada]

For me the patch is ok, but I think it would be better, if somebody else will also review it.

[kiranchandramohan]

Thanks @peixin for this patch.

I have a couple of questions/concerns. See comments inline.

flang/lib/Lower/OpenMP.cpp

511

The barrier is created by the OpenMPIRBuilder. copyprivate's barrier can be handled separately I guess.
https://github.com/llvm/llvm-project/blob/29b37f319ac310e9c023d7c707ecfe1f709807ae/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L3352

flang/test/Lower/OpenMP/single.f90

2

Sorry about the delay here. It is on my list. I will get to it soon.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

241

The single is inlined usually, is the OpenMPIRBuilder behaviour different? We might want to change the name of the Interface if we really want the alloca to be inside the single region. Have you checked the Clang behaviour?

[peixin]

Thanks @kiranchandramohan for pointing out the problem.

You are right. The barrier is emitted in OMPIRBuilder, and the copyprivate should be handled in OMPIRBuilder, too. Clang emits the private/firstprivate in CodeGen (https://github.com/llvm/llvm-project/blob/2188cf9fa4d012b3ce484f9e97b66581569c1157/clang/lib/CodeGen/CGStmtOpenMP.cpp#L4196-L4197) before and after single runtime calls. Also, check the following case:

void sub(float a, double b);
float x = 1.0;
double y = 2.0;
void test() {
  #pragma omp single private(x) firstprivate(y)
  {
    sub(x, y);
  }
}

The generated IR is as follows:

@x = dso_local global float 1.000000e+00, align 4
@y = dso_local global double 2.000000e+00, align 8
@0 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1
@1 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 22, ptr @0 }, align 8
@2 = private unnamed_addr constant %struct.ident_t { i32 0, i32 322, i32 0, i32 22, ptr @0 }, align 8

; Function Attrs: noinline nounwind optnone uwtable
define dso_local void @test() #0 {
entry:
  %y = alloca double, align 8
  %x = alloca float, align 4
  %0 = call i32 @__kmpc_global_thread_num(ptr @1)
  %1 = call i32 @__kmpc_single(ptr @1, i32 %0)
  %2 = icmp ne i32 %1, 0
  br i1 %2, label %omp_if.then, label %omp_if.end

omp_if.then:                                      ; preds = %entry
  %3 = load double, ptr @y, align 8
  store double %3, ptr %y, align 8
  %4 = load float, ptr %x, align 4
  %5 = load double, ptr %y, align 8
  call void @sub(float noundef %4, double noundef %5)
  call void @__kmpc_end_single(ptr @1, i32 %0)
  br label %omp_if.end

omp_if.end:                                       ; preds = %omp_if.then, %entry
  call void @__kmpc_barrier(ptr @2, i32 %0)
  ret void
}

The private/firstprivate are both in single region. So, there is no need to do any additional work in flang lowering for it.

BTW, filed one ticket for one missed sema check for firstprivate clause. https://github.com/llvm/llvm-project/issues/57983

[kiranchandramohan]

In D128596#3814860, @peixin wrote:

Clang emits the private/firstprivate in CodeGen (https://github.com/llvm/llvm-project/blob/2188cf9fa4d012b3ce484f9e97b66581569c1157/clang/lib/CodeGen/CGStmtOpenMP.cpp#L4196-L4197) before and after single runtime calls.

Is it OK to not add the Interface to the Single operation in this case? And keep the interface for operations that are outlined only and reinstating its original name OutlineableOpenMPOpInterface.

[peixin]

In D128596#3817626, @kiranchandramohan wrote:

In D128596#3814860, @peixin wrote:

Clang emits the private/firstprivate in CodeGen (https://github.com/llvm/llvm-project/blob/2188cf9fa4d012b3ce484f9e97b66581569c1157/clang/lib/CodeGen/CGStmtOpenMP.cpp#L4196-L4197) before and after single runtime calls.

Is it OK to not add the Interface to the Single operation in this case? And keep the interface for operations that are outlined only and reinstating its original name OutlineableOpenMPOpInterface.

Of course. The interface is mainly used to get the alloca block currently. The following would work for the single construct.

diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index e1cba68e1686..4f2824f2c591 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -203,6 +203,8 @@ static bool privatizeVars(Op &op, Fortran::lower::AbstractConverter &converter,
   bool needBarrier = false;
   if (mlir::isa<mlir::omp::SectionOp>(op))
     firOpBuilder.setInsertionPointToStart(&op.getRegion().back());
+  else if (mlir::isa<mlir::omp::SingleOp>(op))
+    firOpBuilder.setInsertionPointToStart(&op.getRegion().front());
   else
     firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
   for (auto sym : privatizedSymbols) {

Do you prefer this solution?

[kiranchandramohan]

In D128596#3817707, @peixin wrote:

In D128596#3817626, @kiranchandramohan wrote:

In D128596#3814860, @peixin wrote:

Clang emits the private/firstprivate in CodeGen (https://github.com/llvm/llvm-project/blob/2188cf9fa4d012b3ce484f9e97b66581569c1157/clang/lib/CodeGen/CGStmtOpenMP.cpp#L4196-L4197) before and after single runtime calls.

Is it OK to not add the Interface to the Single operation in this case? And keep the interface for operations that are outlined only and reinstating its original name OutlineableOpenMPOpInterface.

Of course. The interface is mainly used to get the alloca block currently. The following would work for the single construct.

diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index e1cba68e1686..4f2824f2c591 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -203,6 +203,8 @@ static bool privatizeVars(Op &op, Fortran::lower::AbstractConverter &converter,
   bool needBarrier = false;
   if (mlir::isa<mlir::omp::SectionOp>(op))
     firOpBuilder.setInsertionPointToStart(&op.getRegion().back());
+  else if (mlir::isa<mlir::omp::SingleOp>(op))
+    firOpBuilder.setInsertionPointToStart(&op.getRegion().front());
   else
     firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
   for (auto sym : privatizedSymbols) {

Do you prefer this solution?

Why wouldn't getAllocaBlock work by itself? From our understanding of Clang it is not required for the private allocas to be inside the runtime calls, so getAllocaBlock will hoist it to the nearest region that will be outlined. Isn't that sufficient?

[peixin]

Why wouldn't getAllocaBlock work by itself? From our understanding of Clang it is not required for the private allocas to be inside the runtime calls, so getAllocaBlock will hoist it to the nearest region that will be outlined. Isn't that sufficient?

firOpBuilder.getAllocaBlock() is to call getEntryBlock() if it is not OutlineableOpenMPOpInterface, which is to call getFunction().front(). For the following case, it will hoist the private/firstprivate operations outside the parallel region, which is wrong.

subroutine single_private(x, y)
  real :: x
  real(8) :: y

  !$omp parallel
  !$omp single private(x) firstprivate(y)
  call bar(x, y)
  !$omp end single
  !$omp end parallel
end subroutine single_private

[kiranchandramohan]

In D128596#3817748, @peixin wrote:

Why wouldn't getAllocaBlock work by itself? From our understanding of Clang it is not required for the private allocas to be inside the runtime calls, so getAllocaBlock will hoist it to the nearest region that will be outlined. Isn't that sufficient?

firOpBuilder.getAllocaBlock() is to call getEntryBlock() if it is not OutlineableOpenMPOpInterface, which is to call getFunction().front(). For the following case, it will hoist the private/firstprivate operations outside the parallel region, which is wrong.

subroutine single_private(x, y)
  real :: x
  real(8) :: y

  !$omp parallel
  !$omp single private(x) firstprivate(y)
  call bar(x, y)
  !$omp end single
  !$omp end parallel
end subroutine single_private

The code (as given below) tries to get a parent of type OutlineableOpenMPOpInterface which the parallel operation has. If this is not happening then there is a bug.

/// Get the block for adding Allocas.
mlir::Block *fir::FirOpBuilder::getAllocaBlock() {
  auto iface =
      getRegion().getParentOfType<mlir::omp::OutlineableOpenMPOpInterface>();
  return iface ? iface.getAllocaBlock() : getEntryBlock();
}

[peixin]

In D128596#3817758, @kiranchandramohan wrote:

In D128596#3817748, @peixin wrote:

Why wouldn't getAllocaBlock work by itself? From our understanding of Clang it is not required for the private allocas to be inside the runtime calls, so getAllocaBlock will hoist it to the nearest region that will be outlined. Isn't that sufficient?

firOpBuilder.getAllocaBlock() is to call getEntryBlock() if it is not OutlineableOpenMPOpInterface, which is to call getFunction().front(). For the following case, it will hoist the private/firstprivate operations outside the parallel region, which is wrong.

subroutine single_private(x, y)
  real :: x
  real(8) :: y

  !$omp parallel
  !$omp single private(x) firstprivate(y)
  call bar(x, y)
  !$omp end single
  !$omp end parallel
end subroutine single_private

The code (as given below) tries to get a parent of type OutlineableOpenMPOpInterface which the parallel operation has. If this is not happening then there is a bug.

/// Get the block for adding Allocas.
mlir::Block *fir::FirOpBuilder::getAllocaBlock() {
  auto iface =
      getRegion().getParentOfType<mlir::omp::OutlineableOpenMPOpInterface>();
  return iface ? iface.getAllocaBlock() : getEntryBlock();
}

Right, that case works OK. For the following case, the private clause works ok using firOpBuilder.getAllocaBlock(). However, the implementation of firstprivate clause is not reasonable. The alloca ops can be hoisted outside the single region, but the store op for firstprivate clause should be in single region. Clang put all load and store ops in the single region. There is no execution error if put all alloca, load, and store ops outside single region. However, it has worse performance. Actually, moving alloca inside single region will result in less memory usage since it will only allocate one copy of variable for the single thread instead of making the copy for each thread.

 subroutine single_private(x, y)
  real :: x
  real(8) :: y

  !$omp single private(x) firstprivate(y)
  call bar(x, y)
  !$omp end single
end subroutine single_private

Also for the wsloop, the generated IR seems to be not OK, either.

subroutine firstprivate_complex2(arg1, arg2)
        complex(4) :: arg1
        complex(8) :: arg2

!$OMP DO FIRSTPRIVATE(arg1, arg2)
do i = 1,10
        call foo(arg1, arg2)
enddo
!$OMP end DO

end subroutine

func.func @_QPfirstprivate_complex2(%arg0: !fir.ref<!fir.complex<4>> {fir.bindc_name = "arg1"}, %arg1: !fir.ref<!fir.complex<8>> {fir.bindc_name = "arg2"}) {
  %0 = fir.alloca !fir.complex<4> {bindc_name = "arg1", pinned, uniq_name = "_QFfirstprivate_complex2Earg1"}
  %1 = fir.load %arg0 : !fir.ref<!fir.complex<4>>
  fir.store %1 to %0 : !fir.ref<!fir.complex<4>>
  %2 = fir.alloca !fir.complex<8> {bindc_name = "arg2", pinned, uniq_name = "_QFfirstprivate_complex2Earg2"}
  %3 = fir.load %arg1 : !fir.ref<!fir.complex<8>>
  fir.store %3 to %2 : !fir.ref<!fir.complex<8>>
  %4 = fir.alloca i32 {adapt.valuebyref}
  %5 = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFfirstprivate_complex2Ei"}
  %c1_i32 = arith.constant 1 : i32
  %c10_i32 = arith.constant 10 : i32
  %c1_i32_0 = arith.constant 1 : i32
  omp.wsloop   for  (%arg2) : i32 = (%c1_i32) to (%c10_i32) inclusive step (%c1_i32_0) {
    fir.store %arg2 to %4 : !fir.ref<i32>
    fir.call @_QPfoo(%0, %2) : (!fir.ref<!fir.complex<4>>, !fir.ref<!fir.complex<8>>) -> ()
    omp.yield
  }
  return
}

For some loop indexes, there is no need to allocate the memory and perform the firstprivate operations. For example, running do i = 1, 10 using 8 threads in the second round will only need 2 threads. Why do we do it (private/firstprivate) for each thread? The privatization should be after __kmpc_for_static_init and before __kmpc_for_static_fini if in static schedule.

[kiranchandramohan]

In D128596#3817864, @peixin wrote:

In D128596#3817758, @kiranchandramohan wrote:

In D128596#3817748, @peixin wrote:

Why wouldn't getAllocaBlock work by itself? From our understanding of Clang it is not required for the private allocas to be inside the runtime calls, so getAllocaBlock will hoist it to the nearest region that will be outlined. Isn't that sufficient?

firOpBuilder.getAllocaBlock() is to call getEntryBlock() if it is not OutlineableOpenMPOpInterface, which is to call getFunction().front(). For the following case, it will hoist the private/firstprivate operations outside the parallel region, which is wrong.

subroutine single_private(x, y)
  real :: x
  real(8) :: y

  !$omp parallel
  !$omp single private(x) firstprivate(y)
  call bar(x, y)
  !$omp end single
  !$omp end parallel
end subroutine single_private

The code (as given below) tries to get a parent of type OutlineableOpenMPOpInterface which the parallel operation has. If this is not happening then there is a bug.

/// Get the block for adding Allocas.
mlir::Block *fir::FirOpBuilder::getAllocaBlock() {
  auto iface =
      getRegion().getParentOfType<mlir::omp::OutlineableOpenMPOpInterface>();
  return iface ? iface.getAllocaBlock() : getEntryBlock();
}

Right, that case works OK. For the following case, the private clause works ok using firOpBuilder.getAllocaBlock(). However, the implementation of firstprivate clause is not reasonable. The alloca ops can be hoisted outside the single region, but the store op for firstprivate clause should be in single region. Clang put all load and store ops in the single region. There is no execution error if put all alloca, load, and store ops outside single region. However, it has worse performance. Actually, moving alloca inside single region will result in less memory usage since it will only allocate one copy of variable for the single thread instead of making the copy for each thread.

 subroutine single_private(x, y)
  real :: x
  real(8) :: y

  !$omp single private(x) firstprivate(y)
  call bar(x, y)
  !$omp end single
end subroutine single_private

Also for the wsloop, the generated IR seems to be not OK, either.

subroutine firstprivate_complex2(arg1, arg2)
        complex(4) :: arg1
        complex(8) :: arg2

!$OMP DO FIRSTPRIVATE(arg1, arg2)
do i = 1,10
        call foo(arg1, arg2)
enddo
!$OMP end DO

end subroutine

func.func @_QPfirstprivate_complex2(%arg0: !fir.ref<!fir.complex<4>> {fir.bindc_name = "arg1"}, %arg1: !fir.ref<!fir.complex<8>> {fir.bindc_name = "arg2"}) {
  %0 = fir.alloca !fir.complex<4> {bindc_name = "arg1", pinned, uniq_name = "_QFfirstprivate_complex2Earg1"}
  %1 = fir.load %arg0 : !fir.ref<!fir.complex<4>>
  fir.store %1 to %0 : !fir.ref<!fir.complex<4>>
  %2 = fir.alloca !fir.complex<8> {bindc_name = "arg2", pinned, uniq_name = "_QFfirstprivate_complex2Earg2"}
  %3 = fir.load %arg1 : !fir.ref<!fir.complex<8>>
  fir.store %3 to %2 : !fir.ref<!fir.complex<8>>
  %4 = fir.alloca i32 {adapt.valuebyref}
  %5 = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFfirstprivate_complex2Ei"}
  %c1_i32 = arith.constant 1 : i32
  %c10_i32 = arith.constant 10 : i32
  %c1_i32_0 = arith.constant 1 : i32
  omp.wsloop   for  (%arg2) : i32 = (%c1_i32) to (%c10_i32) inclusive step (%c1_i32_0) {
    fir.store %arg2 to %4 : !fir.ref<i32>
    fir.call @_QPfoo(%0, %2) : (!fir.ref<!fir.complex<4>>, !fir.ref<!fir.complex<8>>) -> ()
    omp.yield
  }
  return
}

For some loop indexes, there is no need to allocate the memory and perform the firstprivate operations. For example, running do i = 1, 10 using 8 threads in the second round will only need 2 threads. Why do we do it (private/firstprivate) for each thread? The privatization should be after __kmpc_for_static_init and before __kmpc_for_static_fini if in static schedule.

I think, in general, LLVM recommends for performance reasons to put allocas in the entry block. See https://llvm.org/docs/Frontend/PerformanceTips.html#use-of-allocas.

Yes, there will be some over-allocation. But also note that if we did not hoist and there was a loop outside the single construct then this will lead to run-away allocations without a stacksave and stackrestore.

The case of a loop is a little more complicated, since we need to place the allocas for loops in the header of the loop which is not available in the loop form, it is only available when we break the loop into control-flow form. So it is not possible to do it in the high-level MLIR.

These issues will probably be resolved when we move privatisation clauses to the OpenMP dialect and handle it in the OpenMPIRBuilder.

The alloca ops can be hoisted outside the single region, but the store op for firstprivate clause should be in single region.

Yes, this is important. Otherwise, any updates to the value will not be available. I don't remember whether we fixed the issue or whether it is still present. It was captured in https://github.com/flang-compiler/f18-llvm-project/issues/1171#issuecomment-1119997442

[peixin]

I think, in general, LLVM recommends for performance reasons to put allocas in the entry block. See https://llvm.org/docs/Frontend/PerformanceTips.html#use-of-allocas.

Yes, there will be some over-allocation. But also note that if we did not hoist and there was a loop outside the single construct then this will lead to run-away allocations without a stacksave and stackrestore.

The case of a loop is a little more complicated, since we need to place the allocas for loops in the header of the loop which is not available in the loop form, it is only available when we break the loop into control-flow form. So it is not possible to do it in the high-level MLIR.

These issues will probably be resolved when we move privatisation clauses to the OpenMP dialect and handle it in the OpenMPIRBuilder.

The alloca ops can be hoisted outside the single region, but the store op for firstprivate clause should be in single region.

Yes, this is important. Otherwise, any updates to the value will not be available. I don't remember whether we fixed the issue or whether it is still present. It was captured in https://github.com/flang-compiler/f18-llvm-project/issues/1171#issuecomment-1119997442

I see. Thanks for the explanations. Borrowed some code from D133686 (add @NimishMishra as co-author) for changing the insertion point. Emit the alloca ops at the beginning of the entry block and load/store ops in the single region.

The issue in wsloop hasn't been fixed. It may need to be fixed until the privatization is done in MLIR.

[kiranchandramohan]

LGTM.

flang/lib/Lower/OpenMP.cpp

createBodyOfOp(Op &op, Fortran::lower::AbstractConverter &converter,
   mlir::Block &block = op.getRegion().back();
   firOpBuilder.setInsertionPointToEnd(&block);
 
   // If it is an unstructured region and is not the outer region of a combined
   // construct, create empty blocks for all evaluations.
   if (eval.lowerAsUnstructured() && !outerCombined)
     createEmptyRegionBlocks(firOpBuilder, eval.getNestedEvaluations());
 
+  if constexpr (std::is_same_v<Op, omp::SingleOp>) {
+    // FIXME: Emit an implicit barrier at the end if no 'nowait' clause was
+    // specified and no 'copyprivate' clause.
+    if (!op.nowaitAttr())
+      firOpBuilder.create<mlir::omp::BarrierOp>(loc);
+  }
+
   // Insert the terminator.
   if constexpr (std::is_same_v<Op, omp::WsLoopOp> ||
-                std::is_same_v<Op, omp::SimdLoopOp>) {
-    mlir::ValueRange results;
-    firOpBuilder.create<mlir::omp::YieldOp>(loc, results);
-  } else {
+                std::is_same_v<Op, omp::SimdLoopOp>)
+    firOpBuilder.create<mlir::omp::YieldOp>(loc, mlir::ValueRange());
+  else
     firOpBuilder.create<mlir::omp::TerminatorOp>(loc);
-  }
 
   // Reset the insert point to before the terminator.
   resetBeforeTerminator(firOpBuilder, storeOp, block);
 
   // Handle privatization. Do not privatize if this is the outer operation.
   if (clauses && !outerCombined) {
     bool lastPrivateOp = privatizeVars(op, converter, *clauses, eval);
     // LastPrivatization, due to introduction of
createBodyOfOp<omp::ParallelOp>(parallelOp, converter, currentLocation,
                                     eval, &opClauseList);
   } else if (blockDirective.v == llvm::omp::OMPD_master) {
     auto masterOp =
         firOpBuilder.create<mlir::omp::MasterOp>(currentLocation, argTy);
     createBodyOfOp<omp::MasterOp>(masterOp, converter, currentLocation, eval);
   } else if (blockDirective.v == llvm::omp::OMPD_single) {
     auto singleOp = firOpBuilder.create<mlir::omp::SingleOp>(
         currentLocation, allocateOperands, allocatorOperands, nowaitAttr);
-    createBodyOfOp<omp::SingleOp>(singleOp, converter, currentLocation, eval);
+    createBodyOfOp<omp::SingleOp>(singleOp, converter, currentLocation, eval,
+                                  &opClauseList);
   } else if (blockDirective.v == llvm::omp::OMPD_ordered) {
     auto orderedOp = firOpBuilder.create<mlir::omp::OrderedRegionOp>(
         currentLocation, /*simd=*/nullptr);
     createBodyOfOp<omp::OrderedRegionOp>(orderedOp, converter, currentLocation,
                                          eval);
   } else if (blockDirective.v == llvm::omp::OMPD_task) {
     auto taskOp = firOpBuilder.create<mlir::omp::TaskOp>(
         currentLocation, ifClauseOperand, finalClauseOperand, untiedAttr,
         mergeableAttr, /*in_reduction_vars=*/ValueRange(),
         /*in_reductions=*/nullptr, priorityClauseOperand, allocateOperands,
         allocatorOperands);
     createBodyOfOp(taskOp, converter, currentLocation, eval, &opClauseList);
   } else if (blockDirective.v == llvm::omp::OMPD_taskgroup) {
-      // TODO: Add task_reduction support
     auto taskGroupOp = firOpBuilder.create<mlir::omp::TaskGroupOp>(
         currentLocation, /*task_reduction_vars=*/ValueRange(),
         /*task_reductions=*/nullptr, allocateOperands, allocatorOperands);
     createBodyOfOp(taskGroupOp, converter, currentLocation, eval,
                    &opClauseList);
   } else {
     TODO(converter.getCurrentLocation(), "Unhandled block directive");
   }

flang/test/Lower/OpenMP/default-clause.f90

 !CHECK: %[[temp:.*]] = fir.load %[[X]] : !fir.ref<i32>
 !CHECK: fir.store %[[temp]] to %[[PRIVATE_X]] : !fir.ref<i32>
 !CHECK: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFnested_default_clause_testsEy"}
 !CHECK: %[[temp:.*]] = fir.load %[[Y]] : !fir.ref<i32>
 !CHECK: fir.store %[[temp]] to %[[PRIVATE_Y]] : !fir.ref<i32>
 !CHECK: omp.single {
 !CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
 !CHECK: fir.store %[[temp]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: omp.barrier
 !CHECK: omp.terminator
 !CHECK: }
 !CHECK: omp.terminator
 !CHECK: }
 !CHECK: } 
   !$omp parallel default(firstprivate)
     !$omp single
       x = y
     !$omp end single
   !$omp end parallel
 end subroutine

flang/test/Lower/OpenMP/single.f90

-!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s --check-prefixes="FIRDialect,OMPDialect"
-!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | fir-opt --cfg-conversion | fir-opt --fir-to-llvm-ir | FileCheck %s --check-prefixes="LLVMDialect,OMPDialect"
+!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s
+!RUN: bbc -emit-fir -fopenmp %s -o - | FileCheck %s
 
 !===============================================================================
 ! Single construct
 !===============================================================================
 
-!FIRDialect-LABEL: func @_QPomp_single
-!FIRDialect-SAME: (%[[x:.*]]: !fir.ref<i32> {fir.bindc_name = "x"})
+!CHECK-LABEL: func @_QPomp_single
+!CHECK-SAME: (%[[x:.*]]: !fir.ref<i32> {fir.bindc_name = "x"})
 subroutine omp_single(x)
   integer, intent(inout) :: x
-  !OMPDialect: omp.parallel
+  !CHECK: omp.parallel
   !$omp parallel
-  !OMPDialect: omp.single
+  !CHECK: omp.single
   !$omp single
-    !FIRDialect: %[[xval:.*]] = fir.load %[[x]] : !fir.ref<i32>
-    !FIRDialect: %[[res:.*]] = arith.addi %[[xval]], %{{.*}} : i32
-    !FIRDialect: fir.store %[[res]] to %[[x]] : !fir.ref<i32>
+    !CHECK: %[[xval:.*]] = fir.load %[[x]] : !fir.ref<i32>
+    !CHECK: %[[res:.*]] = arith.addi %[[xval]], %{{.*}} : i32
+    !CHECK: fir.store %[[res]] to %[[x]] : !fir.ref<i32>
     x = x + 12
-  !OMPDialect: omp.terminator
+  !CHECK: omp.barrier
+  !CHECK: omp.terminator
   !$omp end single
-  !OMPDialect: omp.terminator
+  !CHECK: omp.terminator
   !$omp end parallel
 end subroutine omp_single
 
 !===============================================================================
 ! Single construct with nowait
 !===============================================================================
 
-!FIRDialect-LABEL: func @_QPomp_single_nowait
-!FIRDialect-SAME: (%[[x:.*]]: !fir.ref<i32> {fir.bindc_name = "x"})
+!CHECK-LABEL: func @_QPomp_single_nowait
+!CHECK-SAME: (%[[x:.*]]: !fir.ref<i32> {fir.bindc_name = "x"})
 subroutine omp_single_nowait(x)
   integer, intent(inout) :: x
-  !OMPDialect: omp.parallel
+  !CHECK: omp.parallel
   !$omp parallel
-  !OMPDialect: omp.single nowait
+  !CHECK: omp.single nowait
   !$omp single
-    !FIRDialect: %[[xval:.*]] = fir.load %[[x]] : !fir.ref<i32>
-    !FIRDialect: %[[res:.*]] = arith.addi %[[xval]], %{{.*}} : i32
-    !FIRDialect: fir.store %[[res]] to %[[x]] : !fir.ref<i32>
+    !CHECK: %[[xval:.*]] = fir.load %[[x]] : !fir.ref<i32>
+    !CHECK: %[[res:.*]] = arith.addi %[[xval]], %{{.*}} : i32
+    !CHECK: fir.store %[[res]] to %[[x]] : !fir.ref<i32>
     x = x + 12
-  !OMPDialect: omp.terminator
+  !CHECK: omp.terminator
   !$omp end single nowait
-  !OMPDialect: omp.terminator
+  !CHECK: omp.terminator
   !$omp end parallel
 end subroutine omp_single_nowait
 
 !===============================================================================
 ! Single construct with allocate
 !===============================================================================
 
-!FIRDialect-LABEL: func @_QPsingle_allocate
+!CHECK-LABEL: func @_QPsingle_allocate
 subroutine single_allocate()
   use omp_lib
   integer :: x
-  !OMPDialect: omp.parallel {
+  !CHECK: omp.parallel {
   !$omp parallel
-  !OMPDialect: omp.single allocate(
-  !FIRDialect: %{{.+}} : i32 -> %{{.+}} : !fir.ref<i32>
-  !LLVMDialect: %{{.+}} : i32 -> %{{.+}} : !llvm.ptr<i32>
-  !OMPDialect: ) {
+  !CHECK: omp.single allocate(%{{.+}} : i32 -> %{{.+}} : !fir.ref<i32>) {
   !$omp single allocate(omp_high_bw_mem_alloc: x) private(x)
-  !FIRDialect: arith.addi
+  !CHECK: arith.addi
   x = x + 12
-  !OMPDialect: omp.terminator
+  !CHECK: omp.barrier
+  !CHECK: omp.terminator
   !$omp end single
-  !OMPDialect: omp.terminator
+  !CHECK: omp.terminator
   !$omp end parallel
 end subroutine single_allocate
+
+!===============================================================================
+! Single construct with private
+!===============================================================================
+!CHECK-LABEL: func @_QPsingle_private
+subroutine single_private
+  type mytype
+  integer :: x
+  end type mytype
+
+  !CHECK: %[[int_var:.+]] = fir.alloca i32
+  !CHECK: %[[mytype_var:.+]] = fir.alloca !fir.type<_QFsingle_privateTmytype{x:i32}>
+  integer :: int_var
+  type(mytype) :: mytype_var
+
+  !CHECK: fir.call @_QPbar(%[[int_var]], %[[mytype_var]]) : (!fir.ref<i32>, !fir.ref<!fir.type<_QFsingle_privateTmytype{x:i32}>>) -> ()
+  call bar(int_var, mytype_var)
+
+  !CHECK: omp.single {
+  !$omp single private(int_var, mytype_var)
+  !CHECK: %[[int_var_private:.+]] = fir.alloca i32
+  !CHECK: %[[mytype_var_private:.+]] = fir.alloca !fir.type<_QFsingle_privateTmytype{x:i32}>
+
+  !CHECK: fir.call @_QPbar(%[[int_var_private]], %[[mytype_var_private]]) : (!fir.ref<i32>, !fir.ref<!fir.type<_QFsingle_privateTmytype{x:i32}>>) -> ()
+  call bar(int_var, mytype_var)
+  !CHECK: omp.barrier
+  !CHECK: omp.terminator
+  !$omp end single
+end subroutine single_private
+
+!===============================================================================
+! Single construct with firstprivate
+!===============================================================================
+!CHECK-LABEL: func @_QPsingle_firstprivate
+subroutine single_firstprivate
+  type mytype
+  integer :: x
+  end type mytype
+
+  !CHECK: %[[int_var:.+]] = fir.alloca i32
+  !CHECK: %[[mytype_var:.+]] = fir.alloca !fir.type<_QFsingle_firstprivateTmytype{x:i32}>
+  integer :: int_var
+  type(mytype) :: mytype_var
+
+  !CHECK: fir.call @_QPbaz(%[[int_var]], %[[mytype_var]]) : (!fir.ref<i32>, !fir.ref<!fir.type<_QFsingle_firstprivateTmytype{x:i32}>>) -> ()
+  call baz(int_var, mytype_var)
+
+  !CHECK: omp.single {
+  !$omp single firstprivate(int_var, mytype_var)
+  !CHECK: %[[int_var_firstprivate:.+]] = fir.alloca i32
+  !CHECK: %[[int_var_load:.+]] = fir.load %[[int_var]] : !fir.ref<i32>
+  !CHECK: fir.store %[[int_var_load]] to %[[int_var_firstprivate]] : !fir.ref<i32>
+  !CHECK: %[[mytype_var_firstprivate:.+]] = fir.alloca !fir.type<_QFsingle_firstprivateTmytype{x:i32}>
+  !CHECK: %[[mytype_var_load:.+]] = fir.load %[[mytype_var]] : !fir.ref<!fir.type<_QFsingle_firstprivateTmytype{x:i32}>>
+  !CHECK: fir.store %[[mytype_var_load]] to %[[mytype_var_firstprivate]]
+  !CHECK: fir.call @_QPbaz(%[[int_var_firstprivate]], %[[mytype_var_firstprivate]]) : (!fir.ref<i32>, !fir.ref<!fir.type<_QFsingle_firstprivateTmytype{x:i32}>>) -> ()
+  call baz(int_var, mytype_var)
+  !CHECK: omp.barrier
+  !CHECK: omp.terminator
+  !$omp end single
+end subroutine single_firstprivate

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

let extraClassDeclaration = [{
     }
   }];
 }
 
 //===----------------------------------------------------------------------===//
 // 2.8.2 Single Construct
 //===----------------------------------------------------------------------===//
 
-def SingleOp : OpenMP_Op<"single", [AttrSizedOperandSegments]> {
+def SingleOp : OpenMP_Op<"single", [AttrSizedOperandSegments,
+                                    OutlineableOpenMPOpInterface]> {
   let summary = "single directive";
   let description = [{
     The single construct specifies that the associated structured block is
     executed by only one of the threads in the team (not necessarily the
     master thread), in the context of its implicit task. The other threads
     in the team, which do not execute the block, wait at an implicit barrier
     at the end of the single construct unless a nowait clause is specified.
   }];