[flang][OpenMP] Lowering support for default clause

[NimishMishra]

Summary

This patch adds lowering support for default clause.

1. During symbol resolution in semantics, should the enclosing context have a default data sharing clause defined and a parser::Name is not attached to an explicit data sharing clause, the semantics::Symbol::Flag::OmpPrivate flag (in case of default(private)) and semantics::Symbol::Flag::OmpFirstprivate flag (in case of default(firstprivate)) is added to the symbol.

2. During lowering, all symbols having either semantics::Symbol::Flag::OmpPrivate or semantics::Symbol::Flag::OmpFirstprivate flag are collected and privatised appropriately.

Co-authored-by: Peixin Qiao <[e-mail]>

[NimishMishra]

Ping for review!

[shraiysh]

Some minor comments and suggestions. Nice work. Thank you!

flang/lib/Lower/OpenMP.cpp

100

Add hard todo instead of silently returning for unhandled cases.

123

If D125689 gets merged before this, please make sure you add a barrier after firstprivate code generation for default clause. If this patch gets merged first, I will try to handle it in D125689.

800

[nit] Why remove this later? Is there a plan to remove the TODO at the end? I see it as a safety net. Even if all the clauses are implemented, it will protect us from unexpected issues with future new clauses for the time when only parsing has been implemented for them...

flang/lib/Semantics/resolve-directives.cpp

1507

nit: maybe add a hard todo for other values of default clause?

[peixin]

Please rebase this patch.

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

6

Remove the prefix and move them in one line.

[NimishMishra]

flang/lib/Lower/OpenMP.cpp

100

I don't think it's required. If you check the test cases, the behaviour for default(none) and default(shared seems fine. default(none) forces a separate data-sharing clause (for which privatisation is handled elsewhere). default(shared) is, I guess, the default behaviour anyway.

All in all, I don't think we need to do anything else in case of none or shared

800

Thank you. Removed the TODO

[peixin]

CI fails for the two examples in debian.

Flang :: Lower/OpenMP/omp-parallel-wsloop.f90
Flang :: Lower/OpenMP/omp-unstructured.f90

BTW, can you rebase this patch?

[peixin]

For changes in resolve-directives.cpp, can you add one test case for flang-new -fc1 -fdebug-dump-symbols or %python %S/../test_symbols.py %s %flang_fc1 -fopenmp? The examples are flang/test/Semantics/OpenMP/omp-symbol01.f90, omp-do14.f90, ....

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

6

Please remove --check-prefix=FIRDialect. The checks for flang-new and bbc are the same.

[NimishMishra]

Addressed comments

[NimishMishra]

flang/lib/Lower/OpenMP.cpp

118

This accidentally crept in. I will remove this.

[peixin]

flang/lib/Lower/Bridge.cpp

518

Don't understand why you change the previous design. The previous design should work OK for default clause. Which case does not work using the previous design?

flang/lib/Lower/OpenMP.cpp

100

The else is not necessary.

245

Why this? You can still use hasFirstPrivateOp for default clause in some way.

[NimishMishra]

flang/lib/Lower/Bridge.cpp

518

shallowLookupSymbol was not really finding the symbols I had as a result of default clause. I suppose those symbols reside somewhere else other than symbolMap.back() (maybe somewhere down the symbolMap).

flang/lib/Lower/OpenMP.cpp

245

I am inserting an omp.barrier for two cases:

1. firstprivate is present
2. default clause is firstprivate.

Now that I think of it, I should keep it as hasFirstPrivateOp || ompDefaultClauseType == Fortran::parser::OmpDefaultClause::Type::Firstprivate

[peixin]

flang/lib/Lower/Bridge.cpp

518

Can you give me one example (test case)? I can take a look at why it fails and check if it is the problem of the previous design.

flang/lib/Lower/OpenMP.cpp

245

You can change the boolean variable name into better one and add comment for it. To my understanding, these two cases actaully have the same meaning for the data-race problem.

[NimishMishra]

flang/lib/Lower/Bridge.cpp

518

integer :: x, y
!$omp parallel default(firstprivate)
   x = x * y
!$omp end parallel

If for this, I try converter.copyHostAssociateVar(*sym), then I get assertion failure "Host associated symbol box not found".

With the changes I did (basically instead of shallowLookupSymbol, do a separate createHostAssociateVarClone and lookupSymbol), it works.

[kiranchandramohan]

Please add few more tests for nested constructs and with loop inside constructs having default clause.

flang/include/flang/Lower/AbstractConverter.h

106–114

I think exposing so many functions is not appropriate. Do we really need all these?

flang/lib/Lower/OpenMP.cpp

102

At this point, I believe the symbols themselves have the information whether it is private or firstprivate. If so, can we think of combining the genDefaultClause and privatizeVars functions? If there is no default clause present, then also collect all the symbols from the firstprivate, private clauses and hand it here and I believe it will work.

[peixin]

I will take a look at this later this week to check if there is any problem in previous design of copyHostAssociateVar, which may stop its reuse on default clause.

flang/include/flang/Lower/AbstractConverter.h

106–114

+1

flang/lib/Lower/Bridge.cpp

518

Thanks a lot for the detailed information.

flang/lib/Lower/OpenMP.cpp

102

+1

[NimishMishra]

flang/include/flang/Lower/AbstractConverter.h

106–114

shallowLookupSymbol isn't quite working in the case of default clause. I guess if we establish that there's isn't anything I am missing in the implementation and we really need can't use the current copyHostAssociateVar for default clause, probably I can pass on a boolean and modify behaviour inside copyHostAssociateVar itself.

[peixin]

@NimishMishra I tried the following and it works.

diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index 0949cb7d3aea..636307e1d7dc 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -59,43 +59,59 @@ getOmpObjectSymbol(const Fortran::parser::OmpObject &ompObject) {
   return sym;
 }
 
-template <typename T>
-static void createPrivateVarSyms(Fortran::lower::AbstractConverter &converter,
-                                 const T *clause) {
-  const Fortran::parser::OmpObjectList &ompObjectList = clause->v;
-  for (const Fortran::parser::OmpObject &ompObject : ompObjectList.v) {
-    Fortran::semantics::Symbol *sym = getOmpObjectSymbol(ompObject);
-    // Privatization for symbols which are pre-determined (like loop index
-    // variables) happen separately, for everything else privatize here.
-    if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined))
-      continue;
-    bool success = converter.createHostAssociateVarClone(*sym);
-    (void)success;
-    assert(success && "Privatization failed due to existing binding");
-    if constexpr (std::is_same_v<T, Fortran::parser::OmpClause::Firstprivate>) {
-      converter.copyHostAssociateVar(*sym);
-    }
-  }
-}
-
 static void privatizeVars(Fortran::lower::AbstractConverter &converter,
-                          const Fortran::parser::OmpClauseList &opClauseList) {
+                          const Fortran::parser::OmpClauseList &opClauseList,
+                          Fortran::lower::pft::Evaluation &eval) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   auto insPt = firOpBuilder.saveInsertionPoint();
   firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
-  bool hasFirstPrivateOp = false;
+  // Has firstprivate or default(firstprivate) clause?
+  bool hasFirstPrivate = false;
+  // The symbols in private/firstprivate clause or region under
+  // default(private/firstprivate) clause.
+  llvm::SetVector<const Fortran::semantics::Symbol *> symbols;
+
+  // Collect the privatized symbols for private/firstprivate/default clause.
   for (const Fortran::parser::OmpClause &clause : opClauseList.v) {
-    if (const auto &privateClause =
+    auto collectOmpObjectListSymbol = [&](const Fortran::parser::OmpObjectList &ompObjectList) {
+      for (const Fortran::parser::OmpObject &ompObject : ompObjectList.v) {
+        Fortran::semantics::Symbol *sym = getOmpObjectSymbol(ompObject);
+        symbols.insert(sym);
+      }
+    };
+    if (const auto &defaultClause =
+            std::get_if<Fortran::parser::OmpClause::Default>(&clause.u)) {
+      if (defaultClause->v.v == Fortran::parser::OmpDefaultClause::Type::Private)
+        converter.collectSymbolSet(eval, symbols,
+            Fortran::semantics::Symbol::Flag::OmpPrivate, /*isUltimateSymbol=*/false);
+      else if (defaultClause->v.v == Fortran::parser::OmpDefaultClause::Type::Firstprivate)
+        converter.collectSymbolSet(
+            eval, symbols, Fortran::semantics::Symbol::Flag::OmpFirstPrivate,
+            /*isUltimateSymbol=*/false);
+    } else if (const auto &privateClause =
             std::get_if<Fortran::parser::OmpClause::Private>(&clause.u)) {
-      createPrivateVarSyms(converter, privateClause);
+      collectOmpObjectListSymbol(privateClause->v);
     } else if (const auto &firstPrivateClause =
                    std::get_if<Fortran::parser::OmpClause::Firstprivate>(
                        &clause.u)) {
-      createPrivateVarSyms(converter, firstPrivateClause);
-      hasFirstPrivateOp = true;
+      collectOmpObjectListSymbol(firstPrivateClause->v);
+    }
+  }
+
+  for (const Fortran::semantics::Symbol *sym : symbols) {
+    // Privatization for symbols which are pre-determined (like loop index
+    // variables) happen separately, for everything else privatize here.
+    if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined))
+      continue;
+    bool success = converter.createHostAssociateVarClone(*sym);
+    (void)success;
+    assert(success && "Privatization failed due to existing binding");
+    if (sym->test(Fortran::semantics::Symbol::Flag::OmpFirstPrivate)) {
+      converter.copyHostAssociateVar(*sym);
+      hasFirstPrivate = true;
     }
   }
-  if (hasFirstPrivateOp)
+  if (hasFirstPrivate)
     firOpBuilder.create<mlir::omp::BarrierOp>(converter.getCurrentLocation());
   firOpBuilder.restoreInsertionPoint(insPt);
 }
@@ -376,7 +392,7 @@ createBodyOfOp(Op &op, Fortran::lower::AbstractConverter &converter,
 
   // Handle privatization. Do not privatize if this is the outer operation.
   if (clauses && !outerCombined)
-    privatizeVars(converter, *clauses);
+    privatizeVars(converter, *clauses, eval);
 
   if constexpr (std::is_same_v<Op, omp::ParallelOp>) {
     threadPrivatizeVars(converter, eval);

$ cat test.f90 
subroutine sub()
  integer :: x, y
  !$omp parallel default(firstprivate)
    x = x * y
  !$omp end parallel
end
$ bbc -emit-fir -fopenmp test.f90 
$ cat test.mlir 
func.func @_QPsub() {
  %0 = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFsubEx"}
  %1 = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFsubEy"}
  omp.parallel   {
    %2 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFsubEx"}
    %3 = fir.load %0 : !fir.ref<i32>
    fir.store %3 to %2 : !fir.ref<i32>
    %4 = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFsubEy"}
    %5 = fir.load %1 : !fir.ref<i32>
    fir.store %5 to %4 : !fir.ref<i32>
    omp.barrier
    %6 = fir.load %2 : !fir.ref<i32>
    %7 = fir.load %4 : !fir.ref<i32>
    %8 = arith.muli %6, %7 : i32
    fir.store %8 to %2 : !fir.ref<i32>
    omp.terminator
  }
  return
}

Please verify this.

flang/lib/Semantics/resolve-directives.cpp

1498

AddToContextObjectWithDSA(
    *name.symbol, semantics::Symbol::Flag::OmpPrivate);

This seems to be not necessary.

1622

Also remove this if you remove the above.

[NimishMishra]

Addressed comments

[NimishMishra]

The build fails with "502 Bad Gateway". I will restart the build later

[peixin]

Thanks @NimishMishra for the update.

Nothing additional is done for default(none) (which forces defining explicit data sharing clauses, which in turn are anyway handled separately) and for default(shared) (which is anyway the default data-sharing mode).

Nit: In summary, this has been done in another patch. Remove this?

Please wait for one more approval. @kiranchandramohan @shraiysh

[kiranchandramohan]

If you have used code written by @peixin then please add him as a co-author.

Please add a debug-dump-symbols test to check the default symbols.

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

138

Is there no PRIVATE_Y?

143

INNER_PRIVATE_Y?

147–148

Please capture the dataflow here with Named variables.

154–157

Please capture the dataflow here.

175–178

capture the dataflow here or ignore the check lines for the constants.
Consider this as a common comment for all tests.

[NimishMishra]

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

138

No. Variables used in the default(firstprivate) block are not getting captured here. Inside the inner parallel, the host association is being used to create a first-private var.

Should the behaviour be something else?

[kiranchandramohan]

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

138

Could you explain why the treatment of X is different from Y here? Both X and Y are private in the outer and firstprivate in the first inner parallel region right?

[NimishMishra]

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

138

I do not know the exact reason. From playing around, it seems like the default clauses on the outer and inner parallel are getting matched. The fact that x is both inside the inner private and firstprivate block is doing something here. For example, if I remove X from the default(private) inner parallel block, it vanishes from here.

The symbol dump looks something like this (I have removed the other default clauses for brevity):

MainProgram scope: default_clause_lowering size=16 alignment=4
    w size=4 offset=12: ObjectEntity type: INTEGER(4)
    x size=4 offset=0: ObjectEntity type: INTEGER(4)
    y size=4 offset=4: ObjectEntity type: INTEGER(4)
    z size=4 offset=8: ObjectEntity type: INTEGER(4)
    Block scope: size=0 alignment=1
      Block scope: size=0 alignment=1
        x (OmpFirstPrivate): HostAssoc
        y (OmpFirstPrivate): HostAssoc
      Block scope: size=0 alignment=1
        w (OmpPrivate): HostAssoc
        x (OmpPrivate): HostAssoc

Now if we did something like:

!$omp parallel default(firstprivate)
        !$omp parallel default(firstprivate)
            x = y
        !$omp end parallel

        !$omp parallel default(private) shared(z)
            w = x + z
        !$omp end parallel
    !$omp end parallel

the IR generated is

omp.parallel   {
      %4 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
      %5 = fir.load %1 : !fir.ref<i32>
      fir.store %5 to %4 : !fir.ref<i32>
      %6 = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
      %7 = fir.load %2 : !fir.ref<i32>
      fir.store %7 to %6 : !fir.ref<i32>
      omp.barrier
      omp.parallel   {
        %8 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
        %9 = fir.load %4 : !fir.ref<i32>
        fir.store %9 to %8 : !fir.ref<i32>
        %10 = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
        %11 = fir.load %6 : !fir.ref<i32>
        fir.store %11 to %10 : !fir.ref<i32>
        omp.barrier
        %12 = fir.load %10 : !fir.ref<i32>
        fir.store %12 to %8 : !fir.ref<i32>
        omp.terminator
      }
      omp.parallel   {
        %8 = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFEw"}
        %9 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
        %10 = fir.load %9 : !fir.ref<i32>
        %11 = fir.load %3 : !fir.ref<i32>
        %12 = arith.addi %10, %11 : i32
        fir.store %12 to %8 : !fir.ref<i32>
        omp.terminator
      }
      omp.terminator
    }

Note that x is present in both the inner blocks, yet it is first-privatized in the outer parallel.

[NimishMishra]

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

138

Ok, I think the issue is:

if (const auto &privateClause =
                   std::get_if<Fortran::parser::OmpClause::Private>(
                       &clause.u)) {
      collectOmpObjectListSymbol(privateClause->v);
}

So it is capturing default(private) on the outermost block, and gathering symbols which have OmpPrivate only. Hence it is missing OmpFirstPrivate

[NimishMishra]

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

138

Sorry I meant

if (defaultClause->v.v ==
          Fortran::parser::OmpDefaultClause::Type::Private)
        converter.collectSymbolSet(eval, symbols,
                                   Fortran::semantics::Symbol::Flag::OmpPrivate,
                                   /*isUltimateSymbol=*/false);

[peixin]

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

138

Sorry, I missed the special case of the nested constructs and no code between the nested constructs. I think the problem is the following code:

if (GetContext().defaultDSA == semantics::Symbol::Flag::OmpPrivate &&
    !HasDataSharingAttributeObject(*name.symbol)) {
  name.symbol = DeclarePrivateAccessEntity(
      *name.symbol, semantics::Symbol::Flag::OmpPrivate, currScope());
} else if (GetContext().defaultDSA ==
        semantics::Symbol::Flag::OmpFirstPrivate &&
    !HasDataSharingAttributeObject(*name.symbol)) {
  name.symbol = DeclarePrivateAccessEntity(
      *name.symbol, semantics::Symbol::Flag::OmpFirstPrivate, currScope());
}

You should traverse the dirContext to the top and check each level. The dumped symbol should be something like the following:

MainProgram scope: default_clause_lowering size=16 alignment=4
    w size=4 offset=12: ObjectEntity type: INTEGER(4)
    x size=4 offset=0: ObjectEntity type: INTEGER(4)
    y size=4 offset=4: ObjectEntity type: INTEGER(4)
    z size=4 offset=8: ObjectEntity type: INTEGER(4)
    Block scope: size=0 alignment=1
        w (OmpPrivate): HostAssoc
        x (OmpPrivate): HostAssoc
        y (OmpPrivate): HostAssoc
      Block scope: size=0 alignment=1
        x (OmpFirstPrivate): HostAssoc
        y (OmpFirstPrivate): HostAssoc
      Block scope: size=0 alignment=1
        w (OmpPrivate): HostAssoc
        x (OmpPrivate): HostAssoc

When you traverse the dirContext, you should use DeclarePrivateAccessEntity(..., ..., dirScope);. You can get the dirScope by context_.FindScope(dirContext_[i].directiveSource).

Also, you should add one more example for the following:

!$omp parallel default(private)
    !$omp parallel default(firstprivate)
        x = y
    !$omp end parallel

    !$omp parallel default(shared)
        w = x + z
    !$omp end parallel
!$omp end parallel

[peixin]

You may check one more example as follows:

!$omp parallel default(firstprivate)
    !$omp single
        x = y
    !$omp end single
!$omp end parallel

[NimishMishra]

• Addressed comments

• Added support for handling nested block constructs with default clauses

[NimishMishra]

flang/lib/Lower/Bridge.cpp

575

Use of eval here allows all symbols to be collected into a llvm::SetVector, even from the nested constructs. I found two special cases:

!$omp parallel default(private)
    y = 10
    !$omp parallel default(private)
           y = 20
    !$omp end parallel
!$omp end parallel

I got double privatization assertion failures with these. Hence the use of std::map in the current implementation in OpenMP.cpp to keep unique values.

Another corner-case was

!$omp parallel shared(y) default(private)
    y = 10
    !$omp parallel default(private)
         y = 20
    !$omp end parallel
!$omp end parallel

This again looked at the default(...) of the outer construct, and collected all symbols with OmpPrivate. Means even though we have a shared(y), it was getting privatized. For this, I deliberately collect sharedSymbols and remove them from the std::map being privatised.

[peixin]

flang/lib/Lower/Bridge.cpp

559–560

Can you remove this change and add "Depends on D129190" in the summary?

flang/lib/Lower/OpenMP.cpp

167

This code is too fragile. What about the following:

llvm::SetVector<const Fortran::semantics::Symbol *> defaultSymbols;
llvm::SetVector<const Fortran::semantics::Symbol *> symbolsInNestedOmpRegions;
llvm::SetVector<const Fortran::semantics::Symbol *> privatizedSymbols;
// Collect the symbols.
for (auto privatizedSymbol: privatizedSymbols)
  privatizeSymbol(*privatizedSymbol);
for (auto defaultSymbol : defaultSymbols)
  if (!symbolsInNestedRegions.contains(defaultSymbol))
    privatizeSymbol(*defaultSymbol);

flang/lib/Semantics/resolve-directives.cpp

1495

1496

1520

Symbol *hostSymbol =
    defaultDSASymbols.size() ? defaultDSASymbols.back() : &symbol->GetUltimate();
defaultDSASymbols.push_back(DeclarePrivateAccessEntity(
    *hostSymbol, dirContext.defaultDSA,
    context_.FindScope(dirContext.directiveSource)));

[NimishMishra]

flang/lib/Lower/OpenMP.cpp

167

This doesn't solve one problem. Consider:

!$omp parallel default(private)
  x = 20
  !$omp parallel default(firstprivate)
         y = 10
  !$omp end parallel

    !$omp parallel default(first)
         y = 10
  !$omp end parallel
!$omp end parallel

Now defaultSymbols creates the vector [x, y, y]. This causes assertion failure.

symbolsInNestedOmpRegions creates the vector [y, y]. Final privatization thus proceeds only for x because of !symbolsInNestedRegions.contains(defaultSymbol)

[peixin]

I have another doubt. C/C++ does not have default(private | firstprivate). As standard says, the default(firstprivate | private) clause causes all variables in the construct that have implicitly determined data-sharing attributes to be (firstprivate | private). For the following case:

y = 20
!$omp parallel default(private)
  !$omp parallel default(firstprivate)
      y = 10
  !$omp end parallel
!$omp end parallel

Is there a private "y" between default(private) and default(firstprivate)? That is, is y copied from y = 20 or a private y with unknown value? Does y in y = 10 is taken as implicitly determined data-sharing?
What about the following case with the same question:

y = 20
!$omp parallel default(private)
  !$omp parallel firstprivate(y)
      y = 10
  !$omp end parallel
!$omp end parallel

Can you check the behaviors of other compilers?

flang/lib/Lower/OpenMP.cpp

167

defaultSymbols is [x, y1, y2, y3]. symbolsInNestedOmpRegions is [y2, y3].

Block scope: size=0 alignment=1
  x (OmpPrivate): HostAssoc
  y (OmpPrivate): HostAssoc // y1
  Block scope: size=0 alignment=1
    y (OmpFirstPrivate): HostAssoc // y2
  Block scope: size=0 alignment=1
    y (OmpFirstPrivate): HostAssoc // y3

It should work.

[NimishMishra]

flang/lib/Lower/OpenMP.cpp

167

You are right. It works. But it sprouts another problem.

Consider the following:

!$omp parallel default(private)
    !$omp parallel default(private)
         x = 10
   !$omp end parallel
!$omp end parallel

Even with symbolsInNestedRegions, you can see that we still get double privatization in the inner block. Reason being that collectSymbolSet collects both the symbol and the host-associated symbol (here, both have same flags).

I tried to break collectSymbolSet into two: collecting either the symbols or only the host-associated symbols. But that has complicated matters so far.

I'll keep trying and see how this approach can be made to work.

[peixin]

In D123930#3647511, @peixin wrote:

I have another doubt. C/C++ does not have default(private | firstprivate). As standard says, the default(firstprivate | private) clause causes all variables in the construct that have implicitly determined data-sharing attributes to be (firstprivate | private). For the following case:

y = 20
!$omp parallel default(private)
  !$omp parallel default(firstprivate)
      y = 10
  !$omp end parallel
!$omp end parallel

Is there a private "y" between default(private) and default(firstprivate)? That is, is y copied from y = 20 or a private y with unknown value? Does y in y = 10 is taken as implicitly determined data-sharing?
What about the following case with the same question:

y = 20
!$omp parallel default(private)
  !$omp parallel firstprivate(y)
      y = 10
  !$omp end parallel
!$omp end parallel

Can you check the behaviors of other compilers?

Please check and address this comment before going ahead in case we go in the wrong direction.

[kiranchandramohan]

OpenMP 5.1 onwards permits firstprivate/private in C language with some restrictions. https://www.openmp.org/spec-html/5.1/openmpsu116.html#x151-1650002.21.4.1

If you want to test with Clang then you can use a command like ./bin/clang -fopenmp -fopenmp-version=51 -S -emit-llvm

[kiranchandramohan]

@NimishMishra where are we with this? Do you need any help here? Would you be able to reply to @peixin's question and get this ready for submission today?

[NimishMishra]

In D123930#3675689, @kiranchandramohan wrote:

@NimishMishra where are we with this? Do you need any help here? Would you be able to reply to @peixin's question and get this ready for submission today?

I checked this. There is indeed a privatised variable between the outer block with default(...) and the inner block with default(...) in other compilers. This patch also exhibits the same behaviour.

As you would have seen, I use a std::map and some insertions/deletions while lowering to privatise default clause variables. @peixin suggested another approach but that got equally complex (as I mentioned in my previous comment). I have, so far, not been able to arrive at a simpler solution than what we have in this patch.

flang/lib/Lower/OpenMP.cpp

167

You are right. It works. But it sprouts another problem.

Consider the following:

!$omp parallel default(private)
    !$omp parallel default(private)
         x = 10
   !$omp end parallel
!$omp end parallel

Even with symbolsInNestedRegions, you can see that we still get double privatization in the inner block. Reason being that collectSymbolSet collects both the symbol and the host-associated symbol (here, both have same flags).

I tried to break collectSymbolSet into two: collecting either the symbols or only the host-associated symbols. But that has complicated matters so far.

I'll keep trying and see how this approach can be made to work.

[kiranchandramohan]

If nested default clause is the issue, we can consider adding a TODO for that particular case and get the rest of the implementation in. What do you think @NimishMishra @peixin @shraiysh ?
Longer-term I think leaving the expansion of the default clause to private, firstprivate, shared to a pass between semantics and pre-fir tree generation seems to be a possible way. Doing all these traversals and collection of symbols during lowering seems to be mixing issues of frontend and lowering.

[NimishMishra]

In D123930#3677888, @kiranchandramohan wrote:

If nested default clause is the issue, we can consider adding a TODO for that particular case and get the rest of the implementation in. What do you think @NimishMishra @peixin @shraiysh ?
Longer-term I think leaving the expansion of the default clause to private, firstprivate, shared to a pass between semantics and pre-fir tree generation seems to be a possible way. Doing all these traversals and collection of symbols during lowering seems to be mixing issues of frontend and lowering.

Yes. We could do that. You are right in saying that the current implementation mixes up too much of different phases. We should definitely look into reducing that.

[peixin]

If nested default clause is the issue, we can consider adding a TODO for that particular case and get the rest of the implementation in. What do you think @NimishMishra @peixin @shraiysh ?

I agree.

Longer-term I think leaving the expansion of the default clause to private, firstprivate, shared to a pass between semantics and pre-fir tree generation seems to be a possible way. Doing all these traversals and collection of symbols during lowering seems to be mixing issues of frontend and lowering.

I tired to differentiate the symbols (including implicit host-associated or explicit symbols) using the scope. Unfortunately, there is no such thing that I can find during lowering. Adding the scope info in lowering for OpenMP may benefit other constructs/clauses. I haven't tried it in pre-fir. It's maybe worth a try.

@NimishMishra Can you update this patch back to last version with some TODO(s) so not to collect and remove symbols, which is kind of fragile?

[kiranchandramohan]

Is it possible to add a hard TODO for the cases that are not properly handled?

BTW, it will be good to get an approval before merging. In this case, I guess we are in a bit of a hurry and you were probably confused by the messaging from me. Apologies for that.

Also, could you clarify whether you followed @peixin's suggestion or kept the code as it is?

flang/lib/Lower/Bridge.cpp

559–560

Nit: spelling Associated.

[peixin]

BTW, it will be good to get an approval before merging. In this case, I guess we are in a bit of a hurry and you were probably confused by the messaging from me.

+1

flang/include/flang/Lower/AbstractConverter.h

117

This is wrong spelling. I have updated D129190, and you didn't notice it. Also, the comment is not as the function does now.

flang/lib/Lower/OpenMP.cpp

281

You only borrowed the code in D128595 without the description, FIXME and TODO. Also, the summary didn't mention this, and there is not test case for this.

[NimishMishra]

Sorry for the confusion. I mistook it as in we get the implementation in before the code freeze, and later change it to a semantic pass. Should I revert the commit?

Is it possible to add a hard TODO for the cases that are not properly handled?

The problem with the implementation is that it's not good. But it handles the cases we have in the tests here. The other approach we were looking at (collecting the entire evaluations's symbols, collecting only the nested evaluations' symbols, and creating a set difference of the two) gave problems with shared(...) clause. It also gave problems with some other nested constructs which had parent constructs, but did not have children constructs.

BTW, it will be good to get an approval before merging. In this case, I guess we are in a bit of a hurry and you were probably confused by the messaging from me. Apologies for that.

I am sorry. I wait for 2-3 days usually after approval before merging. In this case, I mistook your comment as a go-ahead to submit a working implementation, which we shall change later. I guess there is still some time left before the code freeze, should I revert this commit?

Also, could you clarify whether you followed @peixin's suggestion or kept the code as it is?

That approach did not work with two cases (1. with shared(...) clause and 2. with nested constructs which had parent constructs, but did not have children constructs). The current approach, although fragile, worked with both. So I kept it as it is.

flang/lib/Lower/OpenMP.cpp

281

I did not borrow code from https://reviews.llvm.org/D128595. I was aware of D130027, and I added the change based on that patch and the discussions we had.

Should I revert this completely and begin anew? That might fix the dependencies

[shraiysh]

I have reverted this change for now, we can discuss and add TODOs for the unhandled cases and then merge it.

[kiranchandramohan]

In D123930#3679318, @NimishMishra wrote:

Sorry for the confusion. I mistook it as in we get the implementation in before the code freeze, and later change it to a semantic pass. Should I revert the commit?

BTW, it will be good to get an approval before merging. In this case, I guess we are in a bit of a hurry and you were probably confused by the messaging from me. Apologies for that.

I am sorry. I wait for 2-3 days usually after approval before merging. In this case, I mistook your comment as a go-ahead to submit a working implementation, which we shall change later. I guess there is still some time left before the code freeze, should I revert this commit?

That is alright. The semantics implementation is a longer-term suggestion. You don't have to do it in this patch.

Also, could you clarify whether you followed @peixin's suggestion or kept the code as it is?

That approach did not work with two cases (1. with shared(...) clause and 2. with nested constructs which had parent constructs, but did not have children constructs). The current approach, although fragile, worked with both. So I kept it as it is.

I would prefer a hard TODO for the nested default but if that is not required then it is OK.
Could you fix the minor spelling nit and the comments from Peixin and then submit it with an approval from @shraiysh or @peixin today? (Don't wait for me)

[shraiysh]

In D123930#3679457, @kiranchandramohan wrote:

Could you fix the minor spelling nit and the comments from Peixin and then submit it with an approval from @shraiysh or @peixin today? (Don't wait for me)

Given that @peixin has more context for this revision, it would be faster if you could review the addressing of comments for this @peixin.

If Peixin is not online/available then I will try to review it if the patch updates tonight.

[peixin]

It is at 23:30 now in my time zone. So, I am afraid that I cannot review it after the update.

I didn't review the approach of collecting and removing symbols of the current patch in detail since I feel like this is not a good way. Can we use the approach of https://reviews.llvm.org/D123930#3661846 and add TODO for the cases of double privitization? This is not one hard comment if @shraiysh help review the current approach.

[kiranchandramohan]

@NimishMishra Do you have a plan on how to go forward with this? I am away for a couple of weeks. Could you discuss with peixin and shraiysh either here or in tomorrows call and make progress and submit?

[NimishMishra]

In D123930#3682443, @kiranchandramohan wrote:

@NimishMishra Do you have a plan on how to go forward with this? I am away for a couple of weeks. Could you discuss with peixin and shraiysh either here or in tomorrows call and make progress and submit?

Yes. I plan to discuss further steps on this in tomorrow's call and close at the earliest.

[NimishMishra]

This latest update closely follows @peixin's suggestions and avoids std::map and inserting/deleting symbols. Because of this, one test that was earlier passing is now failing. I have added the same in flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90. Rest all nested constructs are passing.

[NimishMishra]

flang/lib/Lower/OpenMP.cpp

281

@peixin Sorry for the confusion with https://reviews.llvm.org/D128595. Should I bring those changes in here? Or will you move ahead with it separately? Apologies again for the confusion

[peixin]

flang/lib/Lower/OpenMP.cpp

281

Don't worry about that. I have abandoned it and won't work on that again. You can move those changes in this patch.

[peixin]

flang/include/flang/Lower/AbstractConverter.h

113

Can you merge these two functions into one by adding one more argument to collectSymbolSet since they are quite similar?

flang/lib/Lower/OpenMP.cpp

188

189

[NimishMishra]

Merged the two symbol collector methods into one

[peixin]

flang/include/flang/Lower/AbstractConverter.h

111

Not correct comment.

117

The default value of false sounds to be more reasonable.

flang/lib/Lower/Bridge.cpp

564

flang/lib/Lower/OpenMP.cpp

63–64

This function needs to be removed.

96

This needs to be moved in line 185. This is confusing since privatizedSymbols is used for private/firstprivate clause instead of default clause.

99

There may not be one pre-FIR pass to address this issue. How about the following:

FIXME: Collect the symbols with private/firstprivate flag in the region of the construct with default(private/firstprivate) clause excluding the symbols with the same private/firstprivate flag in the inner nested regions.

100

190

The "{" is not necessary.

208

Remove the blank line.

306–308

converter.collectSymbolSet(eval, threadprivateSyms,
                           Fortran::semantics::Symbol::Flag::OmpThreadprivate);

Use this since this uses the default behavior of collectSymbolSet, and no need to specify these two arguments.

flang/lib/Semantics/resolve-directives.cpp

1495

This comment is missed.

1496

This comment is missed.

1520

This comment is missed.

1523

[NimishMishra]

Addressed comments.

flang/lib/Lower/OpenMP.cpp

63–64

This function is being used by the last private clause inside privatizeVars

[peixin]

Thanks for the update. LGTM. You can wait for a day or two to land this if there is no further comments.

flang/lib/Lower/OpenMP.cpp

108

Remove this since this for loop is not only for private/firstprivate clause.

[peixin]

Can you land this so that other privatization-related patches can be implemented based on this?

[NimishMishra]

I have landed the patch with https://reviews.llvm.org/rG435feefbdd6c91faf24fa5e69c4e7c3bc127568a.

Thank you @peixin, @kiranchandramohan, @shraiysh and others for working through this review.

flang/include/flang/Lower/AbstractConverter.h

public:
   /// Get the code defined by a label
   virtual pft::Evaluation *lookupLabel(pft::Label label) = 0;
 
   /// For a given symbol which is host-associated, create a clone using
   /// parameters from the host-associated symbol.
   virtual bool
   createHostAssociateVarClone(const Fortran::semantics::Symbol &sym) = 0;
 
   virtual void copyHostAssociateVar(const Fortran::semantics::Symbol &sym,
                                     mlir::Block *lastPrivBlock = nullptr) = 0;
 
-  /// Collect the set of ultimate symbols of symbols with \p flag in \p eval
-  /// region if \p isUltimateSymbol is true. Otherwise, collect the set of
-  /// symbols with \p flag.
+  /// Collect the set of symbols with \p flag in \p eval
+  /// region if \p collectSymbols is true. Likewise, collect the
+  /// set of the host symbols with \p flag of the associated symbols in \p eval
+  /// region if collectHostAssociatedSymbols is true.
   virtual void collectSymbolSet(
       pft::Evaluation &eval,
       llvm::SetVector<const Fortran::semantics::Symbol *> &symbolSet,
-      Fortran::semantics::Symbol::Flag flag, bool isUltimateSymbol = true) = 0;
+      Fortran::semantics::Symbol::Flag flag, bool collectSymbols = true,
+      bool collectHostAssociatedSymbols = false) = 0;
 
   //===--------------------------------------------------------------------===//
   // Expressions
   //===--------------------------------------------------------------------===//
 
   /// Generate the address of the location holding the expression, \p expr.
   /// If \p expr is a Designator that is not compile time contiguous, the
   /// address returned is the one of a contiguous temporary storage holding the

flang/lib/Lower/Bridge.cpp

copyHostAssociateVar(const Fortran::semantics::Symbol &sym,
     assert(sb && "Host-associated symbol box not found");
     assert(hsb.getAddr() != sb.getAddr() &&
            "Host and associated symbol boxes are the same");
     fir::ExtendedValue exv = getExtendedValue(sb);
 
     // 3) Perform the assignment.
     mlir::OpBuilder::InsertPoint insPt = builder->saveInsertionPoint();
     if (lastPrivBlock)
       builder->setInsertionPointToStart(lastPrivBlock);
     else
       builder->setInsertionPointAfter(fir::getBase(exv).getDefiningOp());
 
     fir::ExtendedValue lhs, rhs;
     if (lastPrivBlock) {
       // lastprivate case
       lhs = hexv;
       rhs = exv;
public:
 
   //===--------------------------------------------------------------------===//
   // Utility methods
   //===--------------------------------------------------------------------===//
 
   void collectSymbolSet(
       Fortran::lower::pft::Evaluation &eval,
       llvm::SetVector<const Fortran::semantics::Symbol *> &symbolSet,
-      Fortran::semantics::Symbol::Flag flag,
-      bool isUltimateSymbol) override final {
+      Fortran::semantics::Symbol::Flag flag, bool collectSymbols,
+      bool checkHostAssociatedSymbols) override final {
     auto addToList = [&](const Fortran::semantics::Symbol &sym) {
-      const Fortran::semantics::Symbol &symbol =
-          isUltimateSymbol ? sym.GetUltimate() : sym;
-      if (symbol.test(flag))
-        symbolSet.insert(&symbol);
+      std::function<void(const Fortran::semantics::Symbol &, bool)>
+          insertSymbols = [&](const Fortran::semantics::Symbol &oriSymbol,
+                              bool collectSymbol) {
+            if (collectSymbol && oriSymbol.test(flag))
+              symbolSet.insert(&oriSymbol);
+            if (checkHostAssociatedSymbols)
+              if (const auto *details{
+                      oriSymbol
+                          .detailsIf<Fortran::semantics::HostAssocDetails>()})
+                insertSymbols(details->symbol(), true);
+          };
+      insertSymbols(sym, collectSymbols);
     };
     Fortran::lower::pft::visitAllSymbols(eval, addToList);
   }
 
   mlir::Location getCurrentLocation() override final { return toLocation(); }
 
   /// Generate a dummy location.
   mlir::Location genUnknownLocation() override final {
     // Note: builder may not be instantiated yet
     return mlir::UnknownLoc::get(&getMLIRContext());

flang/lib/Lower/OpenMP.cpp

std::visit(Fortran::common::visitors{
                      sym = name->symbol;
                    }
                  },
                  [&](const Fortran::parser::Name &name) { sym = name.symbol; }},
              ompObject.u);
   return sym;
 }
 
 template <typename T>
 static void
 createPrivateVarSyms(Fortran::lower::AbstractConverter &converter,
                      const T *clause,
                      [[maybe_unused]] Block *lastPrivBlock = nullptr) {
   const Fortran::parser::OmpObjectList &ompObjectList = clause->v;
   for (const Fortran::parser::OmpObject &ompObject : ompObjectList.v) {
     Fortran::semantics::Symbol *sym = getOmpObjectSymbol(ompObject);
     // Privatization for symbols which are pre-determined (like loop index
     // variables) happen separately, for everything else privatize here.
     if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined))
       continue;
     bool success = converter.createHostAssociateVarClone(*sym);
     (void)success;
     assert(success && "Privatization failed due to existing binding");
     if constexpr (std::is_same_v<T, Fortran::parser::OmpClause::Firstprivate>) {
       converter.copyHostAssociateVar(*sym);
     } else if constexpr (std::is_same_v<
                              T, Fortran::parser::OmpClause::Lastprivate>) {
       converter.copyHostAssociateVar(*sym, lastPrivBlock);
     }
   }
 }
 
 template <typename Op>
 static bool privatizeVars(Op &op, Fortran::lower::AbstractConverter &converter,
-                          const Fortran::parser::OmpClauseList &opClauseList) {
+                          const Fortran::parser::OmpClauseList &opClauseList,
+                          Fortran::lower::pft::Evaluation &eval) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   auto insPt = firOpBuilder.saveInsertionPoint();
   firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
   bool hasFirstPrivateOp = false;
   bool hasLastPrivateOp = false;
+  // Symbols in private and/or firstprivate clauses.
+  llvm::SetVector<const Fortran::semantics::Symbol *> privatizedSymbols;
+  auto collectOmpObjectListSymbol =
+      [&](const Fortran::parser::OmpObjectList &ompObjectList,
+          llvm::SetVector<const Fortran::semantics::Symbol *> &symbolSet) {
+        for (const Fortran::parser::OmpObject &ompObject : ompObjectList.v) {
+          Fortran::semantics::Symbol *sym = getOmpObjectSymbol(ompObject);
+          symbolSet.insert(sym);
+        }
+      };
   // We need just one ICmpOp for multiple LastPrivate clauses.
   mlir::arith::CmpIOp cmpOp;
 
   for (const Fortran::parser::OmpClause &clause : opClauseList.v) {
     if (const auto &privateClause =
             std::get_if<Fortran::parser::OmpClause::Private>(&clause.u)) {
-      createPrivateVarSyms(converter, privateClause);
+      collectOmpObjectListSymbol(privateClause->v, privatizedSymbols);
     } else if (const auto &firstPrivateClause =
                    std::get_if<Fortran::parser::OmpClause::Firstprivate>(
                        &clause.u)) {
-      createPrivateVarSyms(converter, firstPrivateClause);
+      collectOmpObjectListSymbol(firstPrivateClause->v, privatizedSymbols);
       hasFirstPrivateOp = true;
     } else if (const auto &lastPrivateClause =
                    std::get_if<Fortran::parser::OmpClause::Lastprivate>(
                        &clause.u)) {
       // TODO: Add lastprivate support for sections construct, simd construct
       if (std::is_same_v<Op, omp::WsLoopOp>) {
         omp::WsLoopOp *wsLoopOp = dyn_cast<omp::WsLoopOp>(&op);
         fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
         auto insPt = firOpBuilder.saveInsertionPoint();
 
         // Our goal here is to introduce the following control flow
         // just before exiting the worksharing loop.
         // Say our wsloop is as follows:
         //
         // omp.wsloop {
if (const auto &privateClause =
                  "Collapse clause with lastprivate");
         // Only generate the compare once in presence of multiple LastPrivate
         // clauses
         if (!hasLastPrivateOp) {
           cmpOp = firOpBuilder.create<mlir::arith::CmpIOp>(
               wsLoopOp->getLoc(), mlir::arith::CmpIPredicate::eq,
               wsLoopOp->getRegion().front().getArguments()[0],
               wsLoopOp->upperBound()[0]);
         }
         mlir::scf::IfOp ifOp = firOpBuilder.create<mlir::scf::IfOp>(
             wsLoopOp->getLoc(), cmpOp, /*else*/ false);
 
         firOpBuilder.restoreInsertionPoint(insPt);
         createPrivateVarSyms(converter, lastPrivateClause,
                              &(ifOp.getThenRegion().front()));
       } else {
         TODO(converter.getCurrentLocation(),
              "lastprivate clause in constructs other than work-share loop");
       }
       hasLastPrivateOp = true;
     }
   }
+
+  // Symbols in regions with default(private/firstprivate) clause.
+  // FIXME: Collect the symbols with private/firstprivate flag in the region of
+  // the construct with default(private/firstprivate) clause excluding the
+  // symbols with the same private/firstprivate flag in the inner nested
+  // regions.
+  llvm::SetVector<const Fortran::semantics::Symbol *> defaultSymbols;
+  llvm::SetVector<const Fortran::semantics::Symbol *> symbolsInNestedRegions;
+  llvm::SetVector<const Fortran::semantics::Symbol *> symbolsInParentRegions;
+  auto collectSymbols = [&](Fortran::semantics::Symbol::Flag flag) {
+    converter.collectSymbolSet(eval, defaultSymbols, flag,
+                               /*collectSymbols=*/true,
+                               /*collectHostAssociatedSymbols=*/true);
+    for (auto &e : eval.getNestedEvaluations()) {
+      if (e.hasNestedEvaluations())
+        converter.collectSymbolSet(e, symbolsInNestedRegions, flag,
+                                   /*collectSymbols=*/true,
+                                   /*collectHostAssociatedSymbols=*/false);
+      else
+        converter.collectSymbolSet(e, symbolsInParentRegions, flag,
+                                   /*collectSymbols=*/false,
+                                   /*collectHostAssociatedSymbols=*/true);
+    }
+  };
+
+  for (const Fortran::parser::OmpClause &clause : opClauseList.v) {
+    if (const auto &defaultClause =
+            std::get_if<Fortran::parser::OmpClause::Default>(&clause.u)) {
+      if (defaultClause->v.v ==
+          Fortran::parser::OmpDefaultClause::Type::Private)
+        collectSymbols(Fortran::semantics::Symbol::Flag::OmpPrivate);
+      else if (defaultClause->v.v ==
+               Fortran::parser::OmpDefaultClause::Type::Firstprivate)
+        collectSymbols(Fortran::semantics::Symbol::Flag::OmpFirstPrivate);
+    }
+  }
+
+  auto privatizeSymbol = [&](const Fortran::semantics::Symbol *sym) {
+    // Privatization for symbols which are pre-determined (like loop index
+    // variables) happen separately, for everything else privatize here.
+    if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined))
+      return;
+    bool success = converter.createHostAssociateVarClone(*sym);
+    (void)success;
+    assert(success && "Privatization failed due to existing binding");
+    if (sym->test(Fortran::semantics::Symbol::Flag::OmpFirstPrivate)) {
+      converter.copyHostAssociateVar(*sym);
+      hasFirstPrivateOp = true;
+    }
+  };
+
+  for (auto sym : privatizedSymbols)
+    privatizeSymbol(sym);
+
+  for (auto sym : defaultSymbols)
+    if (!symbolsInNestedRegions.contains(sym) &&
+        !symbolsInParentRegions.contains(sym) &&
+        !privatizedSymbols.contains(sym))
+      privatizeSymbol(sym);
   if (hasFirstPrivateOp)
     firOpBuilder.create<mlir::omp::BarrierOp>(converter.getCurrentLocation());
   firOpBuilder.restoreInsertionPoint(insPt);
   return hasLastPrivateOp;
 }
 
 /// The COMMON block is a global structure. \p commonValue is the base address
 /// of the the COMMON block. As the offset from the symbol \p sym, generate the
 /// COMMON block member value (commonValue + offset) for the symbol.
 /// FIXME: Share the code with `instantiateCommon` in ConvertVariable.cpp.
 static mlir::Value
 genCommonBlockMember(Fortran::lower::AbstractConverter &converter,
                      const Fortran::semantics::Symbol &sym,
                      mlir::Value commonValue) {

 static fir::ExtendedValue getExtendedValue(fir::ExtendedValue base,
                                            mlir::Value val) {
   return base.match(
       [&](const fir::MutableBoxValue &box) -> fir::ExtendedValue {
         return fir::MutableBoxValue(val, box.nonDeferredLenParams(), {});
       },
       [&](const auto &) -> fir::ExtendedValue {
         return fir::substBase(base, val);
       });
 }
 
 static void threadPrivatizeVars(Fortran::lower::AbstractConverter &converter,
                                 Fortran::lower::pft::Evaluation &eval) {
   auto &firOpBuilder = converter.getFirOpBuilder();
   mlir::Location currentLocation = converter.getCurrentLocation();
   auto insPt = firOpBuilder.saveInsertionPoint();
   firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
 
   // Get the original ThreadprivateOp corresponding to the symbol and use the
   // symbol value from that opeartion to create one ThreadprivateOp copy
   // operation inside the parallel region.
   auto genThreadprivateOp = [&](Fortran::lower::SymbolRef sym) -> mlir::Value {
     mlir::Value symOriThreadprivateValue = converter.getSymbolAddress(sym);
     mlir::Operation *op = symOriThreadprivateValue.getDefiningOp();
     assert(mlir::isa<mlir::omp::ThreadprivateOp>(op) &&
            "The threadprivate operation not created");
     mlir::Value symValue =
         mlir::dyn_cast<mlir::omp::ThreadprivateOp>(op).sym_addr();
     return firOpBuilder.create<mlir::omp::ThreadprivateOp>(
         currentLocation, symValue.getType(), symValue);
   };
 
   llvm::SetVector<const Fortran::semantics::Symbol *> threadprivateSyms;
-  converter.collectSymbolSet(eval, threadprivateSyms,
-                             Fortran::semantics::Symbol::Flag::OmpThreadprivate,
-                             /*isUltimateSymbol=*/false);
+  converter.collectSymbolSet(
+      eval, threadprivateSyms,
+      Fortran::semantics::Symbol::Flag::OmpThreadprivate);
   std::set<Fortran::semantics::SourceName> threadprivateSymNames;
 
   // For a COMMON block, the ThreadprivateOp is generated for itself instead of
   // its members, so only bind the value of the new copied ThreadprivateOp
   // inside the parallel region to the common block symbol only once for
   // multiple members in one COMMON block.
   llvm::SetVector<const Fortran::semantics::Symbol *> commonSyms;
   for (std::size_t i = 0; i < threadprivateSyms.size(); i++) {
if constexpr (std::is_same_v<Op, omp::WsLoopOp> ||
     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);
+    bool lastPrivateOp = privatizeVars(op, converter, *clauses, eval);
     // LastPrivatization, due to introduction of
     // new control flow, changes the insertion point,
     // thus restore it.
     // TODO: Clean up later a bit to avoid this many sets and resets.
     if (lastPrivateOp)
       resetBeforeTerminator(firOpBuilder, storeOp, block);
   }
 
if (const auto &ifClause =
         continue;
       }
     } else if (std::get_if<Fortran::parser::OmpClause::Threads>(&clause.u)) {
       // Nothing needs to be done for threads clause.
       continue;
     } else if (const auto &finalClause =
                    std::get_if<Fortran::parser::OmpClause::Final>(&clause.u)) {
       mlir::Value finalVal = fir::getBase(converter.genExprValue(
           *Fortran::semantics::GetExpr(finalClause->v), stmtCtx));
       finalClauseOperand = firOpBuilder.createConvert(
           currentLocation, firOpBuilder.getI1Type(), finalVal);
     } else if (std::get_if<Fortran::parser::OmpClause::Untied>(&clause.u)) {
       untiedAttr = firOpBuilder.getUnitAttr();
     } else if (std::get_if<Fortran::parser::OmpClause::Mergeable>(&clause.u)) {
       mergeableAttr = firOpBuilder.getUnitAttr();
     } else if (const auto &priorityClause =
                    std::get_if<Fortran::parser::OmpClause::Priority>(

flang/lib/Semantics/resolve-directives.cpp

if (!symbol->owner().IsDerivedType() && !symbol->has<ProcEntityDetails>() &&
         } else if (GetContext().defaultDSA == Symbol::Flag::OmpNone) {
           context_.Say(name.source,
               "The DEFAULT(NONE) clause requires that '%s' must be listed in "
               "a data-sharing attribute clause"_err_en_US,
               symbol->name());
         }
       }
     }
+    std::vector<Symbol *> defaultDSASymbols;
+    for (int dirDepth{0}; dirDepth < (int)dirContext_.size(); ++dirDepth) {
+      DirContext &dirContext = dirContext_[dirDepth];
+      bool hasDataSharingAttr{false};
+      for (auto symMap : dirContext.objectWithDSA) {
+        // if the `symbol` already has a data-sharing attribute
+        if (symMap.first->name() == name.symbol->name()) {
+          hasDataSharingAttr = true;
+          break;
+        }
+      }
+      if (hasDataSharingAttr) {
+        if (defaultDSASymbols.size())
+          symbol = &MakeAssocSymbol(symbol->name(), *defaultDSASymbols.back(),
+              context_.FindScope(dirContext.directiveSource));
+        continue;
+      }
+
+      if (dirContext.defaultDSA == semantics::Symbol::Flag::OmpPrivate ||
+          dirContext.defaultDSA == semantics::Symbol::Flag::OmpFirstPrivate) {
+        Symbol *hostSymbol = defaultDSASymbols.size() ? defaultDSASymbols.back()
+                                                      : &symbol->GetUltimate();
+        defaultDSASymbols.push_back(
+            DeclarePrivateAccessEntity(*hostSymbol, dirContext.defaultDSA,
+                context_.FindScope(dirContext.directiveSource)));
+      } else if (defaultDSASymbols.size())
+        symbol = &MakeAssocSymbol(symbol->name(), *defaultDSASymbols.back(),
+            context_.FindScope(dirContext.directiveSource));
+    }
   } // 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;
   } else {
     return nullptr;
common::visit(
                 if (dataCopyingAttributeFlags.test(ompFlag)) {
                   CheckDataCopyingClause(*name, *symbol, ompFlag);
                 } else {
                   AddToContextObjectWithDSA(*symbol, ompFlag);
                   if (dataSharingAttributeFlags.test(ompFlag)) {
                     CheckMultipleAppearances(*name, *symbol, ompFlag);
                   }
                   if (privateDataSharingAttributeFlags.test(ompFlag)) {
                     CheckObjectInNamelist(*name, *symbol, ompFlag);
                   }
 
                   if (ompFlag == Symbol::Flag::OmpAllocate) {
                     AddAllocateName(name);
                   }
                 }
                 if (ompFlag == Symbol::Flag::OmpDeclarativeAllocateDirective &&
                     IsAllocatable(*symbol)) {

flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90

+! This test checks the lowering of parallel do
+
+! RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s
+! RUN: bbc -fopenmp -emit-fir %s -o - | FileCheck %s
+
+! The string "EXPECTED" denotes the expected FIR
+
+! CHECK: omp.parallel   {
+! EXPECTED: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
+! EXPECTED: %[[PRIVATE_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFEz"}
+! CHECK: %[[TEMP:.*]] = fir.alloca i32 {adapt.valuebyref, pinned}
+! CHECK: %[[const_1:.*]] = arith.constant 1 : i32
+! CHECK: %[[const_2:.*]] = arith.constant 10 : i32
+! CHECK: %[[const_3:.*]] = arith.constant 1 : i32
+! CHECK: omp.wsloop   for  (%[[ARG:.*]]) : i32 = (%[[const_1]]) to (%[[const_2]]) inclusive step (%[[const_3]]) {
+! CHECK: fir.store %[[ARG]] to %[[TEMP]] : !fir.ref<i32>
+! EXPECTED: %[[temp_1:.*]] = fir.load %[[PRIVATE_Z]] : !fir.ref<i32>
+! CHECK: %[[temp_1:.*]] = fir.load %{{.*}} : !fir.ref<i32>
+! CHECK: %[[temp_2:.*]] = fir.load %[[TEMP]] : !fir.ref<i32>
+! CHECK: %[[result:.*]] = arith.addi %[[temp_1]], %[[temp_2]] : i32
+! EXPECTED: fir.store %[[result]] to %[[PRIVATE_Y]] : !fir.ref<i32>
+! CHECK: fir.store %[[result]] to %{{.*}} : !fir.ref<i32>
+! CHECK: omp.yield
+! CHECK: }
+! CHECK: omp.terminator
+! CHECK: }
+subroutine nested_default_clause()
+  integer x, y, z
+  !$omp parallel do default(private)
+    do x = 1, 10
+      y = z + x
+    enddo
+  !$omp end parallel do
+end subroutine

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

+! This test checks lowering of OpenMP parallel directive
+! with `DEFAULT` clause present.
+
+! RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s
+! RUN: bbc -fopenmp -emit-fir %s -o - | FileCheck %s
+
+
+!CHECK: func @_QQmain() {
+!CHECK: %[[W:.*]] = fir.alloca i32 {bindc_name = "w", uniq_name = "_QFEw"}
+!CHECK: %[[X:.*]] = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFEx"}
+!CHECK: %[[Y:.*]] = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFEy"}
+!CHECK: %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFEz"}
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
+!CHECK: %[[const:.*]] = fir.load %[[X]] : !fir.ref<i32>
+!CHECK: fir.store %[[const]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
+!CHECK: %[[PRIVATE_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFEw"}
+!CHECK: omp.barrier
+!CHECK: %[[const:.*]] = arith.constant 2 : i32
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: %[[result:.*]] = arith.muli %[[const]], %[[temp]] : i32
+!CHECK: fir.store %[[result]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_W]] : !fir.ref<i32>
+!CHECK: %[[const:.*]] = arith.constant 45 : i32
+!CHECK: %[[result:.*]] = arith.addi %[[temp]], %[[const]] : i32
+!CHECK: fir.store %[[result]] to %[[Z]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+
+program default_clause_lowering
+    integer :: x, y, z, w
+
+    !$omp parallel default(private) firstprivate(x) shared(z)
+        x = y * 2
+        z = w + 45
+    !$omp end parallel
+
+!CHECK: omp.parallel {
+!CHECK: %[[temp:.*]] = fir.load %[[Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[X]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+
+    !$omp parallel default(shared)
+        x = y
+    !$omp end parallel
+
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
+!CHECK: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+
+    !$omp parallel default(none) private(x, y)
+        x = y
+    !$omp end parallel
+
+!CHECK: omp.parallel   {
+!CHECK: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
+!CHECK: %[[temp:.*]] = fir.load %[[Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
+!CHECK: %[[temp:.*]] = fir.load %[[X]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: omp.barrier
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+
+    !$omp parallel default(firstprivate) firstprivate(y)
+        x = y
+    !$omp end parallel
+
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
+!CHECK: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
+!CHECK: %[[temp:.*]] = fir.load %[[Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: %[[PRIVATE_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFEw"}
+!CHECK: %[[temp:.*]] = fir.load %[[W]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_W]] : !fir.ref<i32>
+!CHECK: omp.barrier
+!CHECK: %[[const:.*]] = arith.constant 2 : i32
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: %[[result:.*]] = arith.muli %[[const]], %[[temp]] : i32
+!CHECK: fir.store %[[result]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_W]] : !fir.ref<i32>
+!CHECK: %[[const:.*]] = arith.constant 45 : i32
+!CHECK: %[[result:.*]] = arith.addi %[[temp]], %[[const]] : i32
+!CHECK: fir.store %[[result]] to %[[Z]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+
+    !$omp parallel default(firstprivate) private(x) shared(z)
+        x = y * 2
+        z = w + 45
+    !$omp end parallel
+
+!CHECK: omp.parallel   {
+!CHECK: omp.parallel   {
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
+!CHECK: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFEy"}
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFEw"}
+!CHECK: %[[temp:.*]] = fir.load %[[W]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_W]] : !fir.ref<i32>
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFEx"}
+!CHECK: %[[temp:.*]] = fir.load %[[X]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: omp.barrier
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_W]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: omp.terminator
+!CHECK: }
+    !$omp parallel
+        !$omp parallel default(private)
+            x = y
+        !$omp end parallel
+
+        !$omp parallel default(firstprivate)
+            w = x
+        !$omp end parallel
+    !$omp end parallel
+
+end program default_clause_lowering
+
+subroutine nested_default_clause_tests
+    integer :: x, y, z, w, k, a
+    
+!CHECK: %[[K:.*]] = fir.alloca i32 {bindc_name = "k", uniq_name = "_QFnested_default_clause_testsEk"}
+!CHECK: %[[W:.*]] = fir.alloca i32 {bindc_name = "w", uniq_name = "_QFnested_default_clause_testsEw"}
+!CHECK: %[[X:.*]] = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFnested_default_clause_testsEx"}
+!CHECK: %[[Y:.*]] = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFnested_default_clause_testsEy"}
+!CHECK: %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFnested_default_clause_testsEz"}
+!CHECK: omp.parallel   {
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
+!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: %[[PRIVATE_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFnested_default_clause_testsEz"}
+!CHECK: %[[PRIVATE_K:.*]] = fir.alloca i32 {bindc_name = "k", pinned, uniq_name = "_QFnested_default_clause_testsEk"}
+!CHECK: omp.barrier
+!CHECK: omp.parallel {
+!CHECK: %[[INNER_PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFnested_default_clause_testsEy"}
+!CHECK: %[[INNER_PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
+!CHECK: %[[const:.*]] = arith.constant 20 : i32
+!CHECK: fir.store %[[const]] to %[[INNER_PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: %[[const:.*]] = arith.constant 10 : i32
+!CHECK: fir.store %[[const]] to %[[INNER_PRIVATE_X]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: omp.parallel   {
+!CHECK: %[[INNER_PRIVATE_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFnested_default_clause_testsEw"}
+!CHECK: %[[INNER_PRIVATE_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFnested_default_clause_testsEz"}
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Z]]
+!CHECK: fir.store %[[temp]] to %[[INNER_PRIVATE_Z]] : !fir.ref<i32>
+!CHECK: %[[INNER_PRIVATE_K:.*]] = fir.alloca i32 {bindc_name = "k", pinned, uniq_name = "_QFnested_default_clause_testsEk"}
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_K]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[INNER_PRIVATE_K]] : !fir.ref<i32>
+!CHECK: omp.barrier
+!CHECK: %[[const:.*]] = arith.constant 30 : i32
+!CHECK: fir.store %[[const]] to %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: %[[const:.*]] = arith.constant 40 : i32
+!CHECK: fir.store %[[const]] to %[[INNER_PRIVATE_W]] : !fir.ref<i32>
+!CHECK: %[[const:.*]] = arith.constant 50 : i32
+!CHECK: fir.store %[[const]] to %[[INNER_PRIVATE_Z]] : !fir.ref<i32>
+!CHECK: %[[const:.*]] = arith.constant 40 : i32
+!CHECK: fir.store %[[const]] to %[[INNER_PRIVATE_K]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: omp.terminator
+!CHECK: }
+    !$omp parallel  firstprivate(x) private(y) shared(w) default(private)  
+        !$omp parallel default(private)
+           y = 20
+           x = 10 
+        !$omp end parallel 
+
+        !$omp parallel default(firstprivate) shared(y) private(w) 
+            y = 30
+            w = 40 
+            z = 50
+            k = 40
+        !$omp end parallel
+    !$omp end parallel
+    
+    
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
+!CHECK: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFnested_default_clause_testsEy"}
+!CHECK: %[[PRIVATE_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFnested_default_clause_testsEz"}
+!CHECK: %[[PRIVATE_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFnested_default_clause_testsEw"} 
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_INNER_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_INNER_X]] : !fir.ref<i32>
+!CHECK: %[[INNER_PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFnested_default_clause_testsEy"}
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[INNER_PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: omp.barrier
+!CHECK: %[[temp:.*]] = fir.load %[[INNER_PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_INNER_X]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_INNER_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFnested_default_clause_testsEw"} 
+!CHECK: %[[PRIVATE_INNER_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
+!CHECK: %[[temp_1:.*]] = fir.load %[[PRIVATE_INNER_X]] : !fir.ref<i32>
+!CHECK: %[[temp_2:.*]] = fir.load %[[PRIVATE_Z]] : !fir.ref<i32>
+!CHECK: %[[result:.*]] = arith.addi %{{.*}}, %{{.*}} : i32
+!CHECK: fir.store %[[result]] to %[[PRIVATE_INNER_W]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+    !$omp parallel default(private)
+        !$omp parallel default(firstprivate)
+            x = y
+        !$omp end parallel
+
+        !$omp parallel default(private) shared(z)
+            w = x + z
+        !$omp end parallel
+    !$omp end parallel    
+    
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
+!CHECK: %[[PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFnested_default_clause_testsEy"}
+!CHECK: %[[PRIVATE_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFnested_default_clause_testsEw"}
+!CHECK: %[[PRIVATE_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFnested_default_clause_testsEz"}
+!CHECK: omp.parallel {
+!CHECK: %[[INNER_PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[INNER_PRIVATE_X]] : !fir.ref<i32>
+!CHECK: %[[INNER_PRIVATE_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFnested_default_clause_testsEy"}
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[INNER_PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: omp.barrier
+!CHECK: %[[temp:.*]] = fir.load %[[INNER_PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[INNER_PRIVATE_X]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: omp.parallel {
+!CHECK: %[[temp_1:.*]] = fir.load %[[PRIVATE_X]] : !fir.ref<i32>
+!CHECK: %[[temp_2:.*]] = fir.load %[[PRIVATE_Z]] : !fir.ref<i32>
+!CHECK: %[[temp_3:.*]] = arith.addi %[[temp_1]], %[[temp_2]] : i32
+!CHECK: fir.store %[[temp_3]] to %[[PRIVATE_W]] : !fir.ref<i32>
+!CHECK: omp.terminator
+!CHECK: }
+!CHECK: }
+    !$omp parallel default(private)
+    !$omp parallel default(firstprivate)
+      x = y
+    !$omp end parallel
+
+    !$omp parallel default(shared)
+      w = x + z
+    !$omp end parallel
+  !$omp end parallel
+
+!CHECK: omp.parallel {
+!CHECK: %[[PRIVATE_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
+!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.barrier
+!CHECK: omp.single {
+!CHECK: %[[temp:.*]] = fir.load %[[PRIVATE_Y]] : !fir.ref<i32>
+!CHECK: fir.store %[[temp]] to %[[PRIVATE_X]] : !fir.ref<i32>
+!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/omp-parallel-lastprivate-clause-scalar.f90

 end do
 !$OMP END DO
 !$OMP END PARALLEL
 print *, arg1, arg2
 end subroutine
 
 !CHECK: func.func @_QPfirstpriv_lastpriv_int(%[[ARG1:.*]]: !fir.ref<i32> {fir.bindc_name = "arg1"}, %[[ARG2:.*]]: !fir.ref<i32> {fir.bindc_name = "arg2"}) {
 !CHECK-DAG: omp.parallel  {
+! Lastprivate Allocation
+!CHECK-NEXT: %[[CLONE2:.*]] = fir.alloca i32 {bindc_name = "arg2"
 !CHECK-DAG: %[[CLONE1:.*]] = fir.alloca i32 {bindc_name = "arg1"
 ! Firstprivate update
 !CHECK-NEXT: %[[FPV_LD:.*]] = fir.load %[[ARG1]] : !fir.ref<i32>
 !CHECK-NEXT: fir.store %[[FPV_LD]] to %[[CLONE1]] : !fir.ref<i32>
-! Lastprivate Allocation
-!CHECK-NEXT: %[[CLONE2:.*]] = fir.alloca i32 {bindc_name = "arg2"
 !CHECK-NEXT: omp.barrier
 !CHECK: omp.wsloop for (%[[INDX_WS:.*]]) : {{.*}} {
 
 ! Testing last iteration check
 !CHECK-DAG: %[[IV_CMP1:.*]] = arith.cmpi eq, %[[INDX_WS]]
 !CHECK-DAG: scf.if %[[IV_CMP1]] {
 ! Testing lastprivate val update
 !CHECK-NEXT: %[[CLONE_LD:.*]] = fir.load %[[CLONE2]] : !fir.ref<i32>