-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Flang][OpenMP] Skip component symbols when creating extended values #79657
Conversation
The `map` clause in OpenMP allows structure components to be specified (unlike other clauses). Structure components do get their own symbols, but these are not meant to be instantiated. When a component reference is passed as an argument to the omp.target op, it gets a corresponding parameter in the target op's entry block. The original symbols are then bound to the same kind of an extended value as before, but the value is now based on the parameters. To handle structure components more gracefully, put their symbols on the list of mapped objects, but skip them when creating extended values. Fixes llvm#79478.
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesThe When a component reference is passed as an argument to the omp.target op, it gets a corresponding parameter in the target op's entry block. The original symbols are then bound to the same kind of an extended value as before, but the value is now based on the parameters. To handle structure components more gracefully, put their symbols on the list of mapped objects, but skip them when creating extended values. Fixes #79478. Full diff: https://github.com/llvm/llvm-project/pull/79657.diff 3 Files Affected:
diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index d2215f4d1bf1ce3..e11d189e969e6d4 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -58,6 +58,9 @@ getOmpObjectSymbol(const Fortran::parser::OmpObject &ompObject) {
Fortran::parser::Unwrap<Fortran::parser::ArrayElement>(
designator)) {
sym = GetFirstName(arrayEle->base).symbol;
+ } else if (auto *structComp = Fortran::parser::Unwrap<
+ Fortran::parser::StructureComponent>(designator)) {
+ sym = structComp->component.symbol;
} else if (const Fortran::parser::Name *name =
Fortran::semantics::getDesignatorNameIfDataRef(
designator)) {
@@ -2743,6 +2746,9 @@ static void genBodyOfTargetOp(
const mlir::BlockArgument &arg = region.getArgument(argIndex);
// Avoid capture of a reference to a structured binding.
const Fortran::semantics::Symbol *sym = argSymbol;
+ // Structure component symbols don't have bindings.
+ if (sym->owner().IsDerivedType())
+ continue;
fir::ExtendedValue extVal = converter.getSymbolExtendedValue(*sym);
extVal.match(
[&](const fir::BoxValue &v) {
diff --git a/flang/test/Lower/OpenMP/FIR/map-component-ref.f90 b/flang/test/Lower/OpenMP/FIR/map-component-ref.f90
new file mode 100644
index 000000000000000..af8c6fa3c5e877e
--- /dev/null
+++ b/flang/test/Lower/OpenMP/FIR/map-component-ref.f90
@@ -0,0 +1,33 @@
+! RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s
+! RUN: bbc -fopenmp -emit-fir %s -o - | FileCheck %s
+
+! CHECK: %[[V0:[0-9]+]] = fir.alloca !fir.type<_QFfooTt0{a0:i32,a1:i32}> {bindc_name = "a", uniq_name = "_QFfooEa"}
+! CHECK: %[[V1:[0-9]+]] = fir.declare %[[V0]] {uniq_name = "_QFfooEa"} : (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>) -> !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>
+! CHECK: %[[V2:[0-9]+]] = fir.field_index a1, !fir.type<_QFfooTt0{a0:i32,a1:i32}>
+! CHECK: %[[V3:[0-9]+]] = fir.coordinate_of %[[V1]], %[[V2]] : (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>, !fir.field) -> !fir.ref<i32>
+! CHECK: %[[V4:[0-9]+]] = omp.map_info var_ptr(%[[V3]] : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "a%a1"}
+! CHECK: %[[V5:[0-9]+]] = omp.map_info var_ptr(%[[V1]] : !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>, !fir.type<_QFfooTt0{a0:i32,a1:i32}>) map_clauses(implicit, tofrom) capture(ByRef) -> !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>> {name = "a"}
+! CHECK: omp.target map_entries(%[[V4]] -> %arg0, %[[V5]] -> %arg1 : !fir.ref<i32>, !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>) {
+! CHECK: ^bb0(%arg0: !fir.ref<i32>, %arg1: !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>):
+! CHECK: %c0_i32 = arith.constant 0 : i32
+! CHECK: %[[V6:[0-9]+]] = fir.declare %arg1 {uniq_name = "_QFfooEa"} : (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>) -> !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>
+! CHECK: %[[V7:[0-9]+]] = fir.field_index a1, !fir.type<_QFfooTt0{a0:i32,a1:i32}>
+! CHECK: %[[V8:[0-9]+]] = fir.coordinate_of %[[V6]], %[[V7]] : (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>, !fir.field) -> !fir.ref<i32>
+! CHECK: fir.store %c0_i32 to %[[V8]] : !fir.ref<i32>
+! CHECK: omp.terminator
+! CHECK: }
+
+subroutine foo()
+ implicit none
+
+ type t0
+ integer :: a0, a1
+ end type
+
+ type(t0) :: a
+
+ !$omp target map(a%a1)
+ a%a1 = 0
+ !$omp end target
+end
+
diff --git a/flang/test/Lower/OpenMP/map-component-ref.f90 b/flang/test/Lower/OpenMP/map-component-ref.f90
new file mode 100644
index 000000000000000..1ed37e73158025b
--- /dev/null
+++ b/flang/test/Lower/OpenMP/map-component-ref.f90
@@ -0,0 +1,31 @@
+! RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+! RUN: bbc -fopenmp -emit-hlfir %s -o - | FileCheck %s
+
+! CHECK: %[[V0:[0-9]+]] = fir.alloca !fir.type<_QFfooTt0{a0:i32,a1:i32}> {bindc_name = "a", uniq_name = "_QFfooEa"}
+! CHECK: %[[V1:[0-9]+]]:2 = hlfir.declare %[[V0]] {uniq_name = "_QFfooEa"} : (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>) -> (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>, !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>)
+! CHECK: %[[V2:[0-9]+]] = hlfir.designate %[[V1]]#0{"a1"} : (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>) -> !fir.ref<i32>
+! CHECK: %[[V3:[0-9]+]] = omp.map_info var_ptr(%[[V2]] : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "a%a1"}
+! CHECK: %[[V4:[0-9]+]] = omp.map_info var_ptr(%[[V1]]#1 : !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>, !fir.type<_QFfooTt0{a0:i32,a1:i32}>) map_clauses(implicit, tofrom) capture(ByRef) -> !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>> {name = "a"}
+! CHECK: omp.target map_entries(%[[V3]] -> %arg0, %[[V4]] -> %arg1 : !fir.ref<i32>, !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>) {
+! CHECK: ^bb0(%arg0: !fir.ref<i32>, %arg1: !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>):
+! CHECK: %[[V5:[0-9]+]]:2 = hlfir.declare %arg1 {uniq_name = "_QFfooEa"} : (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>) -> (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>, !fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>)
+! CHECK: %c0_i32 = arith.constant 0 : i32
+! CHECK: %[[V6:[0-9]+]] = hlfir.designate %[[V5]]#0{"a1"} : (!fir.ref<!fir.type<_QFfooTt0{a0:i32,a1:i32}>>) -> !fir.ref<i32>
+! CHECK: hlfir.assign %c0_i32 to %[[V6]] : i32, !fir.ref<i32>
+! CHECK: omp.terminator
+! CHECK: }
+
+subroutine foo()
+ implicit none
+
+ type t0
+ integer :: a0, a1
+ end type
+
+ type(t0) :: a
+
+ !$omp target map(a%a1)
+ a%a1 = 0
+ !$omp end target
+end
+
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I have a similar fix downstream where I am working on this kind of explicit member mapping right now! :-)
The Linux build passed, Windows build failed due to some spurious error about alleged malware found in an executable generated by cmake... |
That's a weird one I've never seen before, but yeah sounds completely unrelated to this PR. There just appears to be some weird things going on with the windows buildbot that I don't remember occurring when it was ran via Phabricator. Thank you very much for the PR! |
The
map
clause in OpenMP allows structure components to be specified (unlike other clauses). Structure components do get their own symbols, but these are not meant to be instantiated.When a component reference is passed as an argument to the omp.target op, it gets a corresponding parameter in the target op's entry block. The original symbols are then bound to the same kind of an extended value as before, but the value is now based on the parameters.
To handle structure components more gracefully, put their symbols on the list of mapped objects, but skip them when creating extended values.
Fixes #79478.