[flang][OpenMP] Delayed privatization for variables with equivalence association
#100531
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-mlir-openmp Author: Kareem Ergawy (ergawy) Changes… association Handles variables that are storage associated via Full diff: https://github.com/llvm/llvm-project/pull/100531.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 7e76a81e0df92..443979c27c26d 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -523,6 +523,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
fir::ExtendedValue symExV = converter.getSymbolExtendedValue(*sym);
symTable->pushScope();
+ mlir::Type yieldedType;
// Populate the `alloc` region.
{
@@ -542,9 +543,10 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
symTable->addSymbol(*sym, localExV);
symTable->pushScope();
cloneSymbol(sym);
- firOpBuilder.create<mlir::omp::YieldOp>(
+ auto yieldOp = firOpBuilder.create<mlir::omp::YieldOp>(
hsb.getAddr().getLoc(),
symTable->shallowLookupSymbol(*sym).getAddr());
+ yieldedType = yieldOp.getOperand(0).getType();
symTable->popScope();
}
@@ -554,7 +556,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
// First block argument corresponding to the original/host value while
// second block argument corresponding to the privatized value.
mlir::Block *copyEntryBlock = firOpBuilder.createBlock(
- ©Region, /*insertPt=*/{}, {symType, symType}, {symLoc, symLoc});
+ ©Region, /*insertPt=*/{}, {symType, yieldedType}, {symLoc, symLoc});
firOpBuilder.setInsertionPointToEnd(copyEntryBlock);
auto addSymbol = [&](unsigned argIdx, bool force = false) {
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90
new file mode 100644
index 0000000000000..23a3b3f96e3e5
--- /dev/null
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90
@@ -0,0 +1,36 @@
+! Test delayed privatization for variables that are storage associated via `EQUIVALENCE`.
+
+! 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 private_common
+ real x, y
+ equivalence (x,y)
+ !$omp parallel firstprivate(x)
+ x = 3.14
+ !$omp end parallel
+end subroutine
+
+! CHECK: omp.private {type = firstprivate} @[[X_PRIVATIZER:.*]] : ![[X_TYPE:fir.ptr<f32>]] alloc {
+! CHECK: ^bb0(%{{.*}}: ![[X_TYPE]]):
+! CHECK: %[[PRIV_ALLOC:.*]] = fir.alloca f32 {bindc_name = "x", {{.*}}}
+! CHECK: %[[PRIV_DECL:.*]]:2 = hlfir.declare %[[PRIV_ALLOC]] {{{.*}}} : (![[PRIV_TYPE:fir.ref<f32>]]) -> ({{.*}})
+! CHECK: omp.yield(%[[PRIV_DECL]]#0 : ![[PRIV_TYPE]])
+! CHECK: } copy {
+! CHECK: ^bb0(%[[ORIG_PTR:.*]]: ![[X_TYPE]], %[[PRIV_REF:.*]]: ![[PRIV_TYPE]]):
+! CHECK: %[[ORIG_VAL:.*]] = fir.load %[[ORIG_PTR]] : !fir.ptr<f32>
+! CHECK: hlfir.assign %[[ORIG_VAL]] to %[[PRIV_REF]] temporary_lhs : f32, ![[PRIV_TYPE]]
+! CHECK: omp.yield(%[[PRIV_REF]] : ![[PRIV_TYPE]])
+! CHECK: }
+
+! CHECK: func.func @_QPprivate_common() {
+! CHECK: omp.parallel private(@[[X_PRIVATIZER]] %{{.*}}#0 -> %[[PRIV_ARG:.*]] : ![[X_TYPE]]) {
+! CHECK: %[[REG_DECL:.*]]:2 = hlfir.declare %[[PRIV_ARG]] {{{.*}}} : (![[X_TYPE]]) -> ({{.*}})
+! CHECK: %[[CST:.*]] = arith.constant {{.*}}
+! CHECK: hlfir.assign %[[CST]] to %[[REG_DECL]]#0 : {{.*}}
+! CHECK: omp.terminator
+! CHECK: }
+! CHECK: return
+! CHECK: }
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index f5ec5a476ad8f..ffcd3b11a12f3 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2505,7 +2505,11 @@ LogicalResult PrivateClauseOp::verify() {
<< "Did not expect any values to be yielded.";
}
- if (yieldedTypes.size() == 1 && yieldedTypes.front() == symType)
+ // TODO How can we check that 2 types are compatible without leaking
+ // dialect-specific (e.g. FIR) information? The problem is that in some
+ // cases, we would get the input type of the symbol as, e.g. `fir.ptr<i32>`
+ // while the allocated private memory is of type `fir.ref<i32>`.
+ if (yieldedTypes.size() == 1 /*&& yieldedTypes.front() == symType*/)
return success();
auto error = mlir::emitError(yieldOp.getLoc())
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index 9977dd57e3023..d511c063c8d9c 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -2207,13 +2207,14 @@ func.func @omp_distribute_unconstrained_order() -> () {
}
return
}
-// -----
-omp.private {type = private} @x.privatizer : i32 alloc {
-^bb0(%arg0: i32):
- %0 = arith.constant 0.0 : f32
- // expected-error @below {{Invalid yielded value. Expected type: 'i32', got: 'f32'}}
- omp.yield(%0 : f32)
-}
+
+// Disabled for now. See relevant TODO in PrivateClauseOp::verify().
+//omp.private {type = private} @x.privatizer : i32 alloc {
+//^bb0(%arg0: i32):
+// %0 = arith.constant 0.0 : f32
+// // _expected-error_ @below {{Invalid yielded value. Expected type: 'i32', got: 'f32'}}
+// omp.yield(%0 : f32)
+//}
// -----
diff --git a/mlir/test/Target/LLVMIR/openmp-private.mlir b/mlir/test/Target/LLVMIR/openmp-private.mlir
index 5924377581db6..6bc73fc6f102b 100644
--- a/mlir/test/Target/LLVMIR/openmp-private.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-private.mlir
@@ -234,3 +234,38 @@ omp.declare_reduction @reducer.part : !llvm.ptr init {
^bb0(%arg0: !llvm.ptr):
omp.yield
}
+
+// -----
+
+llvm.func @_QPequivalence() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x !llvm.array<4 x i8> : (i64) -> !llvm.ptr
+ %2 = llvm.mlir.constant(0 : index) : i64
+ %3 = llvm.getelementptr %1[0, %2] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.array<4 x i8>
+ omp.parallel private(@_QFequivalenceEx_firstprivate_ptr_f32 %3 -> %arg0 : !llvm.ptr) {
+ %4 = llvm.mlir.constant(3.140000e+00 : f32) : f32
+ llvm.store %4, %arg0 : f32, !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+}
+
+omp.private {type = firstprivate} @_QFequivalenceEx_firstprivate_ptr_f32 : !llvm.ptr alloc {
+^bb0(%arg0: !llvm.ptr):
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x f32 {bindc_name = "x", pinned} : (i64) -> !llvm.ptr
+ omp.yield(%1 : !llvm.ptr)
+} copy {
+^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr):
+ %0 = llvm.load %arg0 : !llvm.ptr -> f32
+ llvm.store %0, %arg1 : f32, !llvm.ptr
+ omp.yield(%arg1 : !llvm.ptr)
+}
+
+// CHECK: define internal void @_QPequivalence..omp_par
+// CHECK: %[[PRIV_ALLOC:.*]] = alloca float, i64 1, align 4
+// CHECK: %[[HOST_VAL:.*]] = load float, ptr %{{.*}}, align 4
+// Test that we initialzie the firstprivate variable.
+// CHECK: store float %[[HOST_VAL]], ptr %[[PRIV_ALLOC]], align 4
+// Test that we inlined the body of the parallel region.
+// CHECK: store float 0x{{.*}}, ptr %[[PRIV_ALLOC]], align 4
|
|
@llvm/pr-subscribers-mlir-llvm Author: Kareem Ergawy (ergawy) Changes… association Handles variables that are storage associated via Full diff: https://github.com/llvm/llvm-project/pull/100531.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 7e76a81e0df92..443979c27c26d 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -523,6 +523,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
fir::ExtendedValue symExV = converter.getSymbolExtendedValue(*sym);
symTable->pushScope();
+ mlir::Type yieldedType;
// Populate the `alloc` region.
{
@@ -542,9 +543,10 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
symTable->addSymbol(*sym, localExV);
symTable->pushScope();
cloneSymbol(sym);
- firOpBuilder.create<mlir::omp::YieldOp>(
+ auto yieldOp = firOpBuilder.create<mlir::omp::YieldOp>(
hsb.getAddr().getLoc(),
symTable->shallowLookupSymbol(*sym).getAddr());
+ yieldedType = yieldOp.getOperand(0).getType();
symTable->popScope();
}
@@ -554,7 +556,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
// First block argument corresponding to the original/host value while
// second block argument corresponding to the privatized value.
mlir::Block *copyEntryBlock = firOpBuilder.createBlock(
- ©Region, /*insertPt=*/{}, {symType, symType}, {symLoc, symLoc});
+ ©Region, /*insertPt=*/{}, {symType, yieldedType}, {symLoc, symLoc});
firOpBuilder.setInsertionPointToEnd(copyEntryBlock);
auto addSymbol = [&](unsigned argIdx, bool force = false) {
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90
new file mode 100644
index 0000000000000..23a3b3f96e3e5
--- /dev/null
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90
@@ -0,0 +1,36 @@
+! Test delayed privatization for variables that are storage associated via `EQUIVALENCE`.
+
+! 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 private_common
+ real x, y
+ equivalence (x,y)
+ !$omp parallel firstprivate(x)
+ x = 3.14
+ !$omp end parallel
+end subroutine
+
+! CHECK: omp.private {type = firstprivate} @[[X_PRIVATIZER:.*]] : ![[X_TYPE:fir.ptr<f32>]] alloc {
+! CHECK: ^bb0(%{{.*}}: ![[X_TYPE]]):
+! CHECK: %[[PRIV_ALLOC:.*]] = fir.alloca f32 {bindc_name = "x", {{.*}}}
+! CHECK: %[[PRIV_DECL:.*]]:2 = hlfir.declare %[[PRIV_ALLOC]] {{{.*}}} : (![[PRIV_TYPE:fir.ref<f32>]]) -> ({{.*}})
+! CHECK: omp.yield(%[[PRIV_DECL]]#0 : ![[PRIV_TYPE]])
+! CHECK: } copy {
+! CHECK: ^bb0(%[[ORIG_PTR:.*]]: ![[X_TYPE]], %[[PRIV_REF:.*]]: ![[PRIV_TYPE]]):
+! CHECK: %[[ORIG_VAL:.*]] = fir.load %[[ORIG_PTR]] : !fir.ptr<f32>
+! CHECK: hlfir.assign %[[ORIG_VAL]] to %[[PRIV_REF]] temporary_lhs : f32, ![[PRIV_TYPE]]
+! CHECK: omp.yield(%[[PRIV_REF]] : ![[PRIV_TYPE]])
+! CHECK: }
+
+! CHECK: func.func @_QPprivate_common() {
+! CHECK: omp.parallel private(@[[X_PRIVATIZER]] %{{.*}}#0 -> %[[PRIV_ARG:.*]] : ![[X_TYPE]]) {
+! CHECK: %[[REG_DECL:.*]]:2 = hlfir.declare %[[PRIV_ARG]] {{{.*}}} : (![[X_TYPE]]) -> ({{.*}})
+! CHECK: %[[CST:.*]] = arith.constant {{.*}}
+! CHECK: hlfir.assign %[[CST]] to %[[REG_DECL]]#0 : {{.*}}
+! CHECK: omp.terminator
+! CHECK: }
+! CHECK: return
+! CHECK: }
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index f5ec5a476ad8f..ffcd3b11a12f3 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2505,7 +2505,11 @@ LogicalResult PrivateClauseOp::verify() {
<< "Did not expect any values to be yielded.";
}
- if (yieldedTypes.size() == 1 && yieldedTypes.front() == symType)
+ // TODO How can we check that 2 types are compatible without leaking
+ // dialect-specific (e.g. FIR) information? The problem is that in some
+ // cases, we would get the input type of the symbol as, e.g. `fir.ptr<i32>`
+ // while the allocated private memory is of type `fir.ref<i32>`.
+ if (yieldedTypes.size() == 1 /*&& yieldedTypes.front() == symType*/)
return success();
auto error = mlir::emitError(yieldOp.getLoc())
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index 9977dd57e3023..d511c063c8d9c 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -2207,13 +2207,14 @@ func.func @omp_distribute_unconstrained_order() -> () {
}
return
}
-// -----
-omp.private {type = private} @x.privatizer : i32 alloc {
-^bb0(%arg0: i32):
- %0 = arith.constant 0.0 : f32
- // expected-error @below {{Invalid yielded value. Expected type: 'i32', got: 'f32'}}
- omp.yield(%0 : f32)
-}
+
+// Disabled for now. See relevant TODO in PrivateClauseOp::verify().
+//omp.private {type = private} @x.privatizer : i32 alloc {
+//^bb0(%arg0: i32):
+// %0 = arith.constant 0.0 : f32
+// // _expected-error_ @below {{Invalid yielded value. Expected type: 'i32', got: 'f32'}}
+// omp.yield(%0 : f32)
+//}
// -----
diff --git a/mlir/test/Target/LLVMIR/openmp-private.mlir b/mlir/test/Target/LLVMIR/openmp-private.mlir
index 5924377581db6..6bc73fc6f102b 100644
--- a/mlir/test/Target/LLVMIR/openmp-private.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-private.mlir
@@ -234,3 +234,38 @@ omp.declare_reduction @reducer.part : !llvm.ptr init {
^bb0(%arg0: !llvm.ptr):
omp.yield
}
+
+// -----
+
+llvm.func @_QPequivalence() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x !llvm.array<4 x i8> : (i64) -> !llvm.ptr
+ %2 = llvm.mlir.constant(0 : index) : i64
+ %3 = llvm.getelementptr %1[0, %2] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.array<4 x i8>
+ omp.parallel private(@_QFequivalenceEx_firstprivate_ptr_f32 %3 -> %arg0 : !llvm.ptr) {
+ %4 = llvm.mlir.constant(3.140000e+00 : f32) : f32
+ llvm.store %4, %arg0 : f32, !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+}
+
+omp.private {type = firstprivate} @_QFequivalenceEx_firstprivate_ptr_f32 : !llvm.ptr alloc {
+^bb0(%arg0: !llvm.ptr):
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x f32 {bindc_name = "x", pinned} : (i64) -> !llvm.ptr
+ omp.yield(%1 : !llvm.ptr)
+} copy {
+^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr):
+ %0 = llvm.load %arg0 : !llvm.ptr -> f32
+ llvm.store %0, %arg1 : f32, !llvm.ptr
+ omp.yield(%arg1 : !llvm.ptr)
+}
+
+// CHECK: define internal void @_QPequivalence..omp_par
+// CHECK: %[[PRIV_ALLOC:.*]] = alloca float, i64 1, align 4
+// CHECK: %[[HOST_VAL:.*]] = load float, ptr %{{.*}}, align 4
+// Test that we initialzie the firstprivate variable.
+// CHECK: store float %[[HOST_VAL]], ptr %[[PRIV_ALLOC]], align 4
+// Test that we inlined the body of the parallel region.
+// CHECK: store float 0x{{.*}}, ptr %[[PRIV_ALLOC]], align 4
|
ergawy
changed the title
equivalence…equivalence association
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ergawy
changed the title
equivalence associationequivalence association
ergawy
force-pushed
the
delayed_privatization_equivalence
branch
from
5252b29 to
a8a797f
Compare
Dinistro
reviewed
Jul 25, 2024
tblah
reviewed
Jul 25, 2024
ergawy
force-pushed
the
delayed_privatization_equivalence
branch
from
a8a797f to
48ea3cc
Compare
|
@kiranchandramohan @Dinistro Thanks for the review, do you have any further comments/suggestions on this PR? |
I'm fine with the implemented solution and do not have the expertise to properly review the flang parts. If this looks good to others, it's good for me 🙂 |
ergawy
force-pushed
the
delayed_privatization_equivalence
branch
from
48ea3cc to
17265b4
Compare
skatrak
approved these changes
Jul 31, 2024
ergawy
force-pushed
the
delayed_privatization_equivalence
branch
from
282aa1d to
73855c0
Compare
… association Handles variables that are storage associated via `equivalence`. The problem is that these variables are declared as `fir.ptr`s while their privatized storage is declared as `fir.ref` which was triggering a validation error in the OpenMP dialect.
ergawy
force-pushed
the
delayed_privatization_equivalence
branch
from
73855c0 to
174abf6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Handles variables that are storage associated via
equivalence. The problem is that these variables are declared asfir.ptrs while their privatized storage is declared asfir.refwhich was triggering a validation error in the OpenMP dialect.