[flang][MLIR][OpenMP] Extend delayed privatization for arrays and characters #85023

ergawy · 2024-03-13T06:10:56Z

One more step in delayed privatization, this PR extends support for
arrays and characters.

llvmbot · 2024-03-13T06:11:26Z

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-fir-hlfir

Author: Kareem Ergawy (ergawy)

Changes

One more step in delayed privatization, this PR extends support for
arrays. In the delayed privatizer, a fir.box_dims operation is emitted
to retrieve the array bounds and extents. The result of these
fir.box_dims and the privatizer argument, are then used to create a
new SymbolBox.

Full diff: https://github.com/llvm/llvm-project/pull/85023.diff

6 Files Affected:

(modified) flang/lib/Lower/Bridge.cpp (+25-11)
(modified) flang/lib/Lower/OpenMP/DataSharingProcessor.cpp (+31-3)
(added) flang/test/Lower/OpenMP/delayed-privatization-allocatable-firstprivate.f90 (+36)
(added) flang/test/Lower/OpenMP/delayed-privatization-allocatable-private.f90 (+43)
(added) flang/test/Lower/OpenMP/delayed-privatization-array.f90 (+75)
(added) flang/test/Lower/OpenMP/delayed-privatization-pointer.f90 (+31)

diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index a668ba4116faab..bc2e2adcb57c7b 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -748,7 +748,8 @@ class FirConverter : public Fortran::lower::AbstractConverter {
 
   void copyVar(mlir::Location loc, mlir::Value dst,
                mlir::Value src) override final {
-    copyVarHLFIR(loc, dst, src);
+    copyVarHLFIR(loc, Fortran::lower::SymbolBox::Intrinsic{dst},
+                 Fortran::lower::SymbolBox::Intrinsic{src});
   }
 
   void copyHostAssociateVar(
@@ -1009,10 +1010,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
       // `omp.private`'s `alloc` block. If this is the case, we return this
       // `SymbolBox::Intrinsic` value.
       if (Fortran::lower::SymbolBox v = symMap->lookupSymbol(sym))
-        return v.match(
-            [&](const Fortran::lower::SymbolBox::Intrinsic &)
-                -> Fortran::lower::SymbolBox { return v; },
-            [](const auto &) -> Fortran::lower::SymbolBox { return {}; });
+        return v;
 
       return {};
     }
@@ -1060,15 +1058,16 @@ class FirConverter : public Fortran::lower::AbstractConverter {
                const Fortran::lower::SymbolBox &rhs_sb) {
     mlir::Location loc = genLocation(sym.name());
     if (lowerToHighLevelFIR())
-      copyVarHLFIR(loc, lhs_sb.getAddr(), rhs_sb.getAddr());
+      copyVarHLFIR(loc, lhs_sb, rhs_sb);
     else
       copyVarFIR(loc, sym, lhs_sb, rhs_sb);
   }
 
-  void copyVarHLFIR(mlir::Location loc, mlir::Value dst, mlir::Value src) {
+  void copyVarHLFIR(mlir::Location loc, Fortran::lower::SymbolBox dst,
+                    Fortran::lower::SymbolBox src) {
     assert(lowerToHighLevelFIR());
-    hlfir::Entity lhs{dst};
-    hlfir::Entity rhs{src};
+    hlfir::Entity lhs{dst.getAddr()};
+    hlfir::Entity rhs{src.getAddr()};
     // Temporary_lhs is set to true in hlfir.assign below to avoid user
     // assignment to be used and finalization to be called on the LHS.
     // This may or may not be correct but mimics the current behaviour
@@ -1082,7 +1081,22 @@ class FirConverter : public Fortran::lower::AbstractConverter {
           /*keepLhsLengthInAllocatableAssignment=*/false,
           /*temporary_lhs=*/true);
     };
-    if (lhs.isAllocatable()) {
+
+    bool isBoxAllocatable = dst.match(
+        [](const fir::MutableBoxValue &box) { return box.isAllocatable(); },
+        [](const fir::FortranVariableOpInterface &box) {
+          return fir::FortranVariableOpInterface(box).isAllocatable();
+        },
+        [](const auto &box) { return false; });
+
+    bool isBoxPointer = dst.match(
+        [](const fir::MutableBoxValue &box) { return box.isPointer(); },
+        [](const fir::FortranVariableOpInterface &box) {
+          return fir::FortranVariableOpInterface(box).isPointer();
+        },
+        [](const auto &box) { return false; });
+
+    if (isBoxAllocatable) {
       // Deep copy allocatable if it is allocated.
       // Note that when allocated, the RHS is already allocated with the LHS
       // shape for copy on entry in createHostAssociateVarClone.
@@ -1097,7 +1111,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
             copyData(lhs, rhs);
           })
           .end();
-    } else if (lhs.isPointer()) {
+    } else if (isBoxPointer) {
       // Set LHS target to the target of RHS (do not copy the RHS
       // target data into the LHS target storage).
       auto loadVal = builder->create<fir::LoadOp>(loc, rhs);
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 717b8cc0276a30..dff5463513000f 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -50,6 +50,7 @@ void DataSharingProcessor::processStep2(mlir::Operation *op, bool isLoop) {
 }
 
 void DataSharingProcessor::insertDeallocs() {
+  // TODO Extend delayed privatization to include a `dealloc` region.
   for (const Fortran::semantics::Symbol *sym : privatizedSymbols)
     if (Fortran::semantics::IsAllocatable(sym->GetUltimate())) {
       converter.createHostAssociateVarCloneDealloc(*sym);
@@ -376,6 +377,7 @@ void DataSharingProcessor::doPrivatize(const Fortran::semantics::Symbol *sym) {
         symLoc, uniquePrivatizerName, symType,
         isFirstPrivate ? mlir::omp::DataSharingClauseType::FirstPrivate
                        : mlir::omp::DataSharingClauseType::Private);
+    fir::ExtendedValue symExV = converter.getSymbolExtendedValue(*sym);
 
     symTable->pushScope();
 
@@ -386,7 +388,31 @@ void DataSharingProcessor::doPrivatize(const Fortran::semantics::Symbol *sym) {
           &allocRegion, /*insertPt=*/{}, symType, symLoc);
 
       firOpBuilder.setInsertionPointToEnd(allocEntryBlock);
-      symTable->addSymbol(*sym, allocRegion.getArgument(0));
+
+      fir::ExtendedValue localBox = symExV.match(
+          [&](const fir::ArrayBoxValue &box) -> fir::ExtendedValue {
+            auto idxTy = firOpBuilder.getIndexType();
+            llvm::SmallVector<mlir::Value> extents;
+            llvm::SmallVector<mlir::Value> lBounds;
+
+            for (unsigned dim = 0; dim < box.getExtents().size(); ++dim) {
+              mlir::Value dimVal =
+                  firOpBuilder.createIntegerConstant(symLoc, idxTy, dim);
+              fir::BoxDimsOp dimInfo = firOpBuilder.create<fir::BoxDimsOp>(
+                  symLoc, idxTy, idxTy, idxTy, allocRegion.getArgument(0),
+                  dimVal);
+              extents.push_back(dimInfo.getExtent());
+              lBounds.push_back(dimInfo.getLowerBound());
+            }
+
+            return fir::ArrayBoxValue(allocRegion.getArgument(0), extents,
+                                      lBounds);
+          },
+          [&](const auto &box) -> fir::ExtendedValue {
+            return fir::substBase(symExV, allocRegion.getArgument(0));
+          });
+
+      symTable->addSymbol(*sym, localBox);
       symTable->pushScope();
       cloneSymbol(sym);
       firOpBuilder.create<mlir::omp::YieldOp>(
@@ -403,10 +429,12 @@ void DataSharingProcessor::doPrivatize(const Fortran::semantics::Symbol *sym) {
       mlir::Block *copyEntryBlock = firOpBuilder.createBlock(
           &copyRegion, /*insertPt=*/{}, {symType, symType}, {symLoc, symLoc});
       firOpBuilder.setInsertionPointToEnd(copyEntryBlock);
-      symTable->addSymbol(*sym, copyRegion.getArgument(0),
+      symTable->addSymbol(*sym,
+                          fir::substBase(symExV, copyRegion.getArgument(0)),
                           /*force=*/true);
       symTable->pushScope();
-      symTable->addSymbol(*sym, copyRegion.getArgument(1));
+      symTable->addSymbol(*sym,
+                          fir::substBase(symExV, copyRegion.getArgument(1)));
       auto ip = firOpBuilder.saveInsertionPoint();
       copyFirstPrivateSymbol(sym, &ip);
 
diff --git a/flang/test/Lower/OpenMP/delayed-privatization-allocatable-firstprivate.f90 b/flang/test/Lower/OpenMP/delayed-privatization-allocatable-firstprivate.f90
new file mode 100644
index 00000000000000..aa835f82b2730b
--- /dev/null
+++ b/flang/test/Lower/OpenMP/delayed-privatization-allocatable-firstprivate.f90
@@ -0,0 +1,36 @@
+! Test delayed privatization for allocatables: `firstprivate`.
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -mmlir --openmp-enable-delayed-privatization \
+! RUN:   -o - %s 2>&1 | FileCheck %s
+! RUN: bbc -emit-hlfir -fopenmp --openmp-enable-delayed-privatization -o - %s 2>&1 |\
+! RUN:   FileCheck %s
+
+subroutine delayed_privatization_allocatable
+  implicit none
+  integer, allocatable :: var1
+
+!$omp parallel firstprivate(var1)
+  var1 = 10
+!$omp end parallel
+end subroutine
+
+! CHECK-LABEL: omp.private {type = firstprivate}
+! CHECK-SAME: @[[PRIVATIZER_SYM:.*]] : [[TYPE:!fir.ref<!fir.box<!fir.heap<i32>>>]] alloc {
+
+! CHECK-NEXT: ^bb0(%[[PRIV_ARG:.*]]: [[TYPE]]):
+
+! CHECK: } copy {
+! CHECK: ^bb0(%[[PRIV_ORIG_ARG:.*]]: [[TYPE]], %[[PRIV_PRIV_ARG:.*]]: [[TYPE]]):
+
+! CHECK-NEXT:  %[[PRIV_BASE_VAL:.*]] = fir.load %[[PRIV_PRIV_ARG]]
+! CHECK-NEXT:  %[[PRIV_BASE_BOX:.*]] = fir.box_addr %[[PRIV_BASE_VAL]]
+! CHECK-NEXT:  %[[PRIV_BASE_ADDR:.*]] = fir.convert %[[PRIV_BASE_BOX]]
+! CHECK-NEXT:  %[[C0:.*]] = arith.constant 0 : i64
+! CHECK-NEXT:  %[[COPY_COND:.*]] = arith.cmpi ne, %[[PRIV_BASE_ADDR]], %[[C0]] : i64
+
+! CHECK-NEXT:  fir.if %[[COPY_COND]] {
+! CHECK-NEXT:    %[[ORIG_BASE_VAL:.*]] = fir.load %[[PRIV_ORIG_ARG]]
+! CHECK-NEXT:    %[[ORIG_BASE_ADDR:.*]] = fir.box_addr %[[ORIG_BASE_VAL]]
+! CHECK-NEXT:    %[[ORIG_BASE_LD:.*]] = fir.load %[[ORIG_BASE_ADDR]]
+! CHECK-NEXT:    hlfir.assign %[[ORIG_BASE_LD]] to %[[PRIV_BASE_BOX]] temporary_lhs
+! CHECK-NEXT:  }
diff --git a/flang/test/Lower/OpenMP/delayed-privatization-allocatable-private.f90 b/flang/test/Lower/OpenMP/delayed-privatization-allocatable-private.f90
new file mode 100644
index 00000000000000..cc1818b00b809c
--- /dev/null
+++ b/flang/test/Lower/OpenMP/delayed-privatization-allocatable-private.f90
@@ -0,0 +1,43 @@
+! Test delayed privatization for allocatables: `private`.
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -mmlir --openmp-enable-delayed-privatization \
+! RUN:   -o - %s 2>&1 | FileCheck %s
+! RUN: bbc -emit-hlfir -fopenmp --openmp-enable-delayed-privatization -o - %s 2>&1 |\
+! RUN:   FileCheck %s
+
+subroutine delayed_privatization_allocatable
+  implicit none
+  integer, allocatable :: var1
+
+!$omp parallel private(var1)
+  var1 = 10
+!$omp end parallel
+end subroutine
+
+! CHECK-LABEL: omp.private {type = private}
+! CHECK-SAME: @[[PRIVATIZER_SYM:.*]] : [[TYPE:!fir.ref<!fir.box<!fir.heap<i32>>>]] alloc {
+
+! CHECK-NEXT: ^bb0(%[[PRIV_ARG:.*]]: [[TYPE]]):
+
+! CHECK-NEXT:   %[[PRIV_ALLOC:.*]] = fir.alloca !fir.box<!fir.heap<i32>> {bindc_name = "var1", pinned, uniq_name = "_QFdelayed_privatization_allocatableEvar1"}
+
+! CHECK-NEXT:   %[[PRIV_ARG_VAL:.*]] = fir.load %[[PRIV_ARG]] : !fir.ref<!fir.box<!fir.heap<i32>>>
+! CHECK-NEXT:   %[[PRIV_ARG_BOX:.*]] = fir.box_addr %[[PRIV_ARG_VAL]] : (!fir.box<!fir.heap<i32>>) -> !fir.heap<i32>
+! CHECK-NEXT:   %[[PRIV_ARG_ADDR:.*]] = fir.convert %[[PRIV_ARG_BOX]] : (!fir.heap<i32>) -> i64
+! CHECK-NEXT:   %[[C0:.*]] = arith.constant 0 : i64
+! CHECK-NEXT:   %[[ALLOC_COND:.*]] = arith.cmpi ne, %[[PRIV_ARG_ADDR]], %[[C0]] : i64
+
+! CHECK-NEXT:   fir.if %[[ALLOC_COND]] {
+! CHECK-NEXT:     %[[PRIV_ALLOCMEM:.*]] = fir.allocmem i32 {fir.must_be_heap = true, uniq_name = "_QFdelayed_privatization_allocatableEvar1.alloc"}
+! CHECK-NEXT:     %[[PRIV_ALLOCMEM_BOX:.*]] = fir.embox %[[PRIV_ALLOCMEM]] : (!fir.heap<i32>) -> !fir.box<!fir.heap<i32>>
+! CHECK-NEXT:     fir.store %[[PRIV_ALLOCMEM_BOX]] to %[[PRIV_ALLOC]] : !fir.ref<!fir.box<!fir.heap<i32>>>
+! CHECK-NEXT:   } else {
+! CHECK-NEXT:     %[[ZERO_BITS:.*]] = fir.zero_bits !fir.heap<i32>
+! CHECK-NEXT:     %[[ZERO_BOX:.*]] = fir.embox %[[ZERO_BITS]] : (!fir.heap<i32>) -> !fir.box<!fir.heap<i32>>
+! CHECK-NEXT:     fir.store %[[ZERO_BOX]] to %[[PRIV_ALLOC]] : !fir.ref<!fir.box<!fir.heap<i32>>>
+! CHECK-NEXT:   }
+
+! CHECK-NEXT:   %[[PRIV_DECL:.*]]:2 = hlfir.declare %[[PRIV_ALLOC]]
+! CHECK-NEXT:   omp.yield(%[[PRIV_DECL]]#0 : [[TYPE]])
+
+! CHECK-NEXT: }
diff --git a/flang/test/Lower/OpenMP/delayed-privatization-array.f90 b/flang/test/Lower/OpenMP/delayed-privatization-array.f90
new file mode 100644
index 00000000000000..608dde412c360e
--- /dev/null
+++ b/flang/test/Lower/OpenMP/delayed-privatization-array.f90
@@ -0,0 +1,75 @@
+! Test delayed privatization for arrays.
+
+! RUN: split-file %s %t
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -mmlir --openmp-enable-delayed-privatization \
+! RUN:   -o - %t/one_dim_array.f90 2>&1 | FileCheck %s --check-prefix=ONE_DIM
+! RUN: bbc -emit-hlfir -fopenmp --openmp-enable-delayed-privatization -o - \
+! RUN:   %t/one_dim_array.f90 2>&1 | FileCheck %s --check-prefix=ONE_DIM
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -mmlir --openmp-enable-delayed-privatization \
+! RUN:   -o - %t/two_dim_array.f90 2>&1 | FileCheck %s --check-prefix=TWO_DIM
+! RUN: bbc -emit-hlfir -fopenmp --openmp-enable-delayed-privatization -o - %t/two_dim_array.f90 2>&1 |\
+! RUN:   FileCheck %s --check-prefix=TWO_DIM
+
+!--- one_dim_array.f90
+subroutine delayed_privatization_private(var1, l1, u1)
+  implicit none
+  integer(8):: l1, u1
+  integer, dimension(l1:u1) :: var1
+
+!$omp parallel firstprivate(var1)
+  var1(l1 + 1) = 10
+!$omp end parallel
+end subroutine
+
+! ONE_DIM-LABEL: omp.private {type = firstprivate}
+! ONE_DIM-SAME: @[[PRIVATIZER_SYM:.*]] : [[TYPE:!fir.box<!fir.array<\?xi32>>]] alloc {
+
+! ONE_DIM-NEXT: ^bb0(%[[PRIV_ARG:.*]]: [[TYPE]]):
+
+! ONE_DIM-NEXT:   %[[C0:.*]] = arith.constant 0 : index
+! ONE_DIM-NEXT:   %[[DIMS:.*]]:3 = fir.box_dims %[[PRIV_ARG]], %[[C0]] : ([[TYPE]], index) -> (index, index, index)
+! ONE_DIM-NEXT:   %[[PRIV_ALLOCA:.*]] = fir.alloca !fir.array<{{\?}}xi32>, %[[DIMS]]#1 {bindc_name = "var1", pinned, uniq_name = "_QFdelayed_privatization_privateEvar1"}
+! ONE_DIM-NEXT:   %[[SHAPE_SHIFT:.*]] = fir.shape_shift %[[DIMS]]#0, %[[DIMS]]#1 : (index, index) -> !fir.shapeshift<1>
+! ONE_DIM-NEXT:   %[[PRIV_DECL:.*]]:2 = hlfir.declare %[[PRIV_ALLOCA]](%[[SHAPE_SHIFT]]) {uniq_name = "_QFdelayed_privatization_privateEvar1"}
+! ONE_DIM-NEXT:  omp.yield(%[[PRIV_DECL]]#0 : [[TYPE]])
+
+! ONE_DIM-NEXT: } copy {
+! ONE_DIM-NEXT: ^bb0(%[[PRIV_ORIG_ARG:.*]]: [[TYPE]], %[[PRIV_PRIV_ARG:.*]]: [[TYPE]]):
+! ONE_DIM-NEXT:  hlfir.assign %[[PRIV_ORIG_ARG]] to %[[PRIV_PRIV_ARG]] temporary_lhs
+! ONE_DIM-NEXT:   omp.yield(%[[PRIV_PRIV_ARG]] : [[TYPE]])
+! ONE_DIM-NEXT: }
+
+!--- two_dim_array.f90
+subroutine delayed_privatization_private(var1, l1, u1, l2, u2)
+  implicit none
+  integer(8):: l1, u1, l2, u2
+  integer, dimension(l1:u1, l2:u2) :: var1
+
+!$omp parallel firstprivate(var1)
+  var1(l1 + 1, u2) = 10
+!$omp end parallel
+end subroutine
+
+! TWO_DIM-LABEL: omp.private {type = firstprivate}
+! TWO_DIM-SAME: @[[PRIVATIZER_SYM:.*]] : [[TYPE:!fir.box<!fir.array<\?x\?xi32>>]] alloc {
+
+! TWO_DIM-NEXT: ^bb0(%[[PRIV_ARG:.*]]: [[TYPE]]):
+! TWO_DIM-NEXT:   %[[C0:.*]] = arith.constant 0 : index
+! TWO_DIM-NEXT:   %[[DIMS0:.*]]:3 = fir.box_dims %[[PRIV_ARG]], %[[C0]] : ([[TYPE]], index) -> (index, index, index)
+
+! TWO_DIM-NEXT:   %[[C1:.*]] = arith.constant 1 : index
+! TWO_DIM-NEXT:   %[[DIMS1:.*]]:3 = fir.box_dims %[[PRIV_ARG]], %[[C1]] : ([[TYPE]], index) -> (index, index, index)
+
+! TWO_DIM-NEXT:   %[[PRIV_ALLOCA:.*]] = fir.alloca !fir.array<{{\?}}xi32>, %[[DIMS0]]#1, %[[DIMS1]]#1 {bindc_name = "var1", pinned, uniq_name = "_QFdelayed_privatization_privateEvar1"}
+! TWO_DIM-NEXT:   %[[SHAPE_SHIFT:.*]] = fir.shape_shift %[[DIMS0]]#0, %[[DIMS0]]#1, %[[DIMS1]]#0, %[[DIMS1]]#1 : (index, index) -> !fir.shapeshift<2>
+
+! TWO_DIM-NEXT:   %[[PRIV_DECL:.*]]:2 = hlfir.declare %[[PRIV_ALLOCA]](%[[SHAPE_SHIFT]]) {uniq_name = "_QFdelayed_privatization_privateEvar1"}
+! TWO_DIM-NEXT:  omp.yield(%[[PRIV_DECL]]#0 : [[TYPE]])
+
+! ONE_DIM-NEXT: } copy {
+! ONE_DIM-NEXT: ^bb0(%[[PRIV_ORIG_ARG:.*]]: [[TYPE]], %[[PRIV_PRIV_ARG:.*]]: [[TYPE]]):
+! ONE_DIM-NEXT:  hlfir.assign %[[PRIV_ORIG_ARG]] to %[[PRIV_PRIV_ARG]] temporary_lhs
+! ONE_DIM-NEXT:   omp.yield(%[[PRIV_PRIV_ARG]] : [[TYPE]])
+! ONE_DIM-NEXT: }
diff --git a/flang/test/Lower/OpenMP/delayed-privatization-pointer.f90 b/flang/test/Lower/OpenMP/delayed-privatization-pointer.f90
new file mode 100644
index 00000000000000..796e4720c8c954
--- /dev/null
+++ b/flang/test/Lower/OpenMP/delayed-privatization-pointer.f90
@@ -0,0 +1,31 @@
+! Test delayed privatization for pointers: `private`.
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -mmlir --openmp-enable-delayed-privatization \
+! RUN:   -o - %s 2>&1 | FileCheck %s
+! RUN: bbc -emit-hlfir -fopenmp --openmp-enable-delayed-privatization -o - %s 2>&1 |\
+! RUN:   FileCheck %s
+
+subroutine delayed_privatization_pointer
+  implicit none
+  integer, pointer :: var1
+
+!$omp parallel firstprivate(var1)
+  var1 = 10
+!$omp end parallel
+end subroutine
+
+! CHECK-LABEL: omp.private {type = firstprivate}
+! CHECK-SAME: @[[PRIVATIZER_SYM:.*]] : [[TYPE:!fir.ref<!fir.box<!fir.ptr<i32>>>]] alloc {
+
+! CHECK-NEXT: ^bb0(%[[PRIV_ARG:.*]]: [[TYPE]]):
+
+! CHECK-NEXT:   %[[PRIV_ALLOC:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "var1", pinned, uniq_name = "_QFdelayed_privatization_pointerEvar1"}
+! CHECK-NEXT:   %[[PRIV_DECL:.*]]:2 = hlfir.declare %[[PRIV_ALLOC]]
+! CHECK-NEXT:   omp.yield(%[[PRIV_DECL]]#0 : [[TYPE]])
+
+! CHECK-NEXT: } copy {
+! CHECK: ^bb0(%[[PRIV_ORIG_ARG:.*]]: [[TYPE]], %[[PRIV_PRIV_ARG:.*]]: [[TYPE]]):
+! CHECK-NEXT:    %[[ORIG_BASE_VAL:.*]] = fir.load %[[PRIV_ORIG_ARG]]
+ ! CHECK-NEXT:   fir.store %[[ORIG_BASE_VAL]] to %[[PRIV_PRIV_ARG]] : !fir.ref<!fir.box<!fir.ptr<i32>>>
+! CHECK-NEXT:   omp.yield(%[[PRIV_PRIV_ARG]] : [[TYPE]])
+! CHECK-NEXT: }

One more step in delayed privatization, this PR extends support for arrays. In the delayed privatizer, a `fir.box_dims` operation is emitted to retrieve the array bounds and extents. The result of these `fir.box_dims` and the privatizer argument, are then used to create a new `ExtendedValue`.

ergawy · 2024-03-18T09:46:54Z

I think this PR is now ready for review. Please have a look when you have time 🙏.

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

jeanPerier · 2024-03-19T15:13:47Z

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

+                                      lBounds);
+          },
+          [&](const auto &box) -> fir::ExtendedValue {
+            return fir::substBase(symExV, allocRegion.getArgument(0));


You will need to handle other cases here (like the array character case (CharArrayBoxValue) and the case for non contiguous arrays (BoxValue)). You can make them explicit TODO here if you are not handling them/testing them in this patch.

An alternative would be to use hlfir::translateToExtendedValue here to get the new ExtendedValue and cover all cases (there are no cleanup returned if the input is an mlir::Value memory reference). But you would need to give it a small hint that the input fir.box is contiguous in the ArrayBoxValue and CharArrayBoxValue cases.

Added explicit todos. I think using hlfir::translateToExtendedValue will not be possible here since we are dealing with the region argument of the privatizer's alloc region which is only an mlir::Value.

An hlfir::Entity is just an mlir::Value with guarantees about what it is. I do not remember how the alloc region value is created, but if it is created from the first result of the original variable hlfir::declare, you can just do hlfir::Entity entity{allocRegion.getArgument(0)}`` and use hlfir::translateToExtendedValue`.

Really appreciate your help. Indeed, after studying what is going on a bit more I now understand more what you mean. I am now using translateToExtendedValue as you suggested.

NimishMishra

Thank you for the patch. I was trying to understand the changes with simpler examples of the form:

program main
    implicit none
    integer :: i
    real, dimension(5) :: array

    !$omp parallel private(array)
       array(2) = array(2) + 10
    !$omp end parallel
end program

It turns out that with -mmlir --openmp-enable-delayed-privatization flag, we get the following verification failure:

error: loc("/home/user1/.llvm/test/par.f90":4:27): 'hlfir.declare' op of array entity with a raw address base must have a shape operand that is a shape or shapeshift
error: loc("/home/user1/.llvm/test/par.f90":4:27): 'fir.shape' op using value defined outside the region
error: loc("/home/user1/.llvm/test/par.f90":4:27): 'hlfir.declare' op of array entity with a raw address base must have a shape operand that is a shape or shapeshift

I am not completely sure what went wrong here.

…on_15

jeanPerier

Sorry, these inlined comments were left pending, I meant to submit them with my answer yesterday.

flang/lib/Optimizer/Builder/HLFIRTools.cpp

jeanPerier · 2024-04-23T08:33:30Z

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

+                                      lBounds);
+          },
+          [&](const auto &box) -> fir::ExtendedValue {
+            return fir::substBase(symExV, allocRegion.getArgument(0));


An hlfir::Entity is just an mlir::Value with guarantees about what it is. I do not remember how the alloc region value is created, but if it is created from the first result of the original variable hlfir::declare, you can just do hlfir::Entity entity{allocRegion.getArgument(0)}`` and use hlfir::translateToExtendedValue`.

…on_15

ergawy · 2024-04-25T04:14:38Z

flang/lib/Optimizer/Builder/HLFIRTools.cpp

@@ -860,7 +859,9 @@ translateVariableToExtendedValue(mlir::Location loc, fir::FirOpBuilder &builder,
                                fir::MutableProperties{});

  if (base.getType().isa<fir::BaseBoxType>()) {
-    if (!variable.isSimplyContiguous() || variable.isPolymorphic() ||
+
+    bool contiguous = variable.isSimplyContiguous() || contiguousHint;


I am not sure about this change yet to be honest. In particular, I am testing the effect of changing this line to: bool contiguous = variable.isSimplyContiguous() || variable.isArray();.

@jeanPerier let me know if you think the above change is a bad idea for any reason.

variable.isArray() would not be correct since this would be true for any array, including x(:) that is not contiguous. Your change using the extra knowledge you have from the symbol/original box makes sense to me.

…on_15

kiranchandramohan

LGTM. Please wait for @jeanPerier @tblah

…on_15

ergawy · 2024-05-03T07:09:24Z

@NimishMishra @jeanPerier @tblah can you folks please have a look and let me know if you have any further comments on this PR?

jeanPerier

Thanks for the update! That looks good to me. One suggested improvement inlined.

jeanPerier · 2024-05-03T14:18:03Z

flang/lib/Optimizer/Builder/HLFIRTools.cpp

@@ -860,7 +859,9 @@ translateVariableToExtendedValue(mlir::Location loc, fir::FirOpBuilder &builder,
                                fir::MutableProperties{});

  if (base.getType().isa<fir::BaseBoxType>()) {
-    if (!variable.isSimplyContiguous() || variable.isPolymorphic() ||
+
+    bool contiguous = variable.isSimplyContiguous() || contiguousHint;


variable.isArray() would not be correct since this would be true for any array, including x(:) that is not contiguous. Your change using the extra knowledge you have from the symbol/original box makes sense to me.

flang/lib/Lower/OpenMP/OpenMP.cpp

…on_15

…aracters (llvm#85023)

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp labels Mar 13, 2024

ergawy force-pushed the delayed_privatization_15 branch 3 times, most recently from 523850e to 0ccde39 Compare March 13, 2024 08:49

ergawy mentioned this pull request Mar 14, 2024

[flang][MLIR][OpenMP] Extend delayed privatization for scalar allocatables and pointers #84740

Merged

ergawy force-pushed the delayed_privatization_15 branch from 0ccde39 to 7e48b5f Compare March 15, 2024 04:19

ergawy requested review from skatrak, jsjodin, kiranchandramohan, abidh, raghavendhra, agozillon, jeanPerier, DominikAdamski, kparzysz, anchuraj, bhandarkar-pranav and TIFitis March 15, 2024 04:21

ergawy mentioned this pull request Mar 15, 2024

[WIP] Delayed privatization. #79862

Closed

ergawy force-pushed the delayed_privatization_15 branch from 7e48b5f to 24f62ad Compare March 18, 2024 09:46

kiranchandramohan requested a review from tblah March 18, 2024 10:25

tblah reviewed Mar 18, 2024

View reviewed changes

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp Outdated Show resolved Hide resolved

jeanPerier reviewed Mar 19, 2024

View reviewed changes

NimishMishra reviewed Mar 21, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

3740156

…on_15

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

df716da

…on_15

jeanPerier reviewed Apr 24, 2024

View reviewed changes

ergawy added 3 commits April 24, 2024 22:45

Use translateToExtendedValue.

738962b

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

bd99c8f

…on_15

clean-ups

080a285

ergawy commented Apr 25, 2024

View reviewed changes

ergawy added 7 commits April 24, 2024 23:56

Extend uses for

0d13dbb

Add one more test.

05a68a6

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

f6cb909

…on_15

More clean-up

1692791

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

2283349

…on_15

Test entities

d71bc1b

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

fe4de72

…on_15

ergawy changed the title ~~[flang][MLIR][OpenMP] Extend delayed privatization for arrays~~ [flang][MLIR][OpenMP] Extend delayed privatization for arrays and characters Apr 29, 2024

More tests for

fb4d40b

ergawy force-pushed the delayed_privatization_15 branch from bf1b7cb to 5578df2 Compare April 29, 2024 07:29

Add tests for character array

0d6aff2

ergawy force-pushed the delayed_privatization_15 branch from 5578df2 to 0d6aff2 Compare April 29, 2024 07:43

ergawy added 2 commits April 29, 2024 04:12

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

c8e44e5

…on_15

fix test error

130dc7a

kiranchandramohan approved these changes Apr 29, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

9a706aa

…on_15

jeanPerier approved these changes May 3, 2024

View reviewed changes

ergawy added 4 commits May 3, 2024 09:28

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

5f8d398

…on_15

use IsSimplyContiguous()

c6a7cfa

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

bb876c1

…on_15

Merge remote-tracking branch 'upstream/main' into delayed_privatizati…

a30ceb3

…on_15

ergawy merged commit 24f5fc7 into llvm:main May 4, 2024
4 checks passed

sookach pushed a commit to sookach/llvm-project that referenced this pull request May 4, 2024

[flang][MLIR][OpenMP] Extend delayed privatization for arrays and ch…

ad7499a

…aracters (llvm#85023)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][MLIR][OpenMP] Extend delayed privatization for arrays and characters #85023

[flang][MLIR][OpenMP] Extend delayed privatization for arrays and characters #85023

ergawy commented Mar 13, 2024 •

edited

llvmbot commented Mar 13, 2024 •

edited

ergawy commented Mar 18, 2024

jeanPerier Mar 19, 2024

ergawy Apr 18, 2024

jeanPerier Apr 23, 2024

ergawy Apr 25, 2024

NimishMishra left a comment

jeanPerier left a comment

jeanPerier Apr 23, 2024

ergawy Apr 25, 2024

jeanPerier May 3, 2024

kiranchandramohan left a comment

ergawy commented May 3, 2024

jeanPerier left a comment

jeanPerier May 3, 2024

[flang][MLIR][OpenMP] Extend delayed privatization for arrays and characters #85023

[flang][MLIR][OpenMP] Extend delayed privatization for arrays and characters #85023

Conversation

ergawy commented Mar 13, 2024 • edited

llvmbot commented Mar 13, 2024 • edited

ergawy commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NimishMishra left a comment

Choose a reason for hiding this comment

jeanPerier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiranchandramohan left a comment

Choose a reason for hiding this comment

ergawy commented May 3, 2024

jeanPerier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ergawy commented Mar 13, 2024 •

edited

llvmbot commented Mar 13, 2024 •

edited