From f0038b4a794c35ea8c7dbf42f783918f2b96b9fb Mon Sep 17 00:00:00 2001 From: ergawy Date: Thu, 25 Jul 2024 02:45:30 -0500 Subject: [PATCH 1/3] [flang][OpenMP] Delayed privatization for variable with `equivalence` 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. --- .../lib/Lower/OpenMP/DataSharingProcessor.cpp | 14 +++++-- .../DelayedPrivatization/equivalence.f90 | 37 +++++++++++++++++++ mlir/test/Target/LLVMIR/openmp-private.mlir | 35 ++++++++++++++++++ 3 files changed, 83 insertions(+), 3 deletions(-) create mode 100644 flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90 diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp index 78f83ede570a6..71c6d1ecb797e 100644 --- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp +++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp @@ -549,9 +549,17 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym, symTable->addSymbol(*sym, localExV); symTable->pushScope(); cloneSymbol(sym); - firOpBuilder.create( - hsb.getAddr().getLoc(), - symTable->shallowLookupSymbol(*sym).getAddr()); + mlir::Value cloneAddr = symTable->shallowLookupSymbol(*sym).getAddr(); + mlir::Type cloneType = cloneAddr.getType(); + + mlir::Value yieldedValue = + (symType == cloneType) ? cloneAddr + : firOpBuilder.createConvert( + cloneAddr.getLoc(), symType, cloneAddr); + + auto yieldOp = firOpBuilder.create( + hsb.getAddr().getLoc(), yieldedValue); + yieldOp.getOperand(0).getType(); symTable->popScope(); } diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90 new file mode 100644 index 0000000000000..1cbbcdcd0e4fd --- /dev/null +++ b/flang/test/Lower/OpenMP/DelayedPrivatization/equivalence.f90 @@ -0,0 +1,37 @@ +! 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]] 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]]) -> ({{.*}}) +! CHECK: %[[PRIV_CONV:.*]] = fir.convert %[[PRIV_DECL]]#0 : (![[PRIV_TYPE]]) -> ![[X_TYPE]] +! CHECK: omp.yield(%[[PRIV_CONV]] : ![[X_TYPE]]) +! CHECK: } copy { +! CHECK: ^bb0(%[[ORIG_PTR:.*]]: ![[X_TYPE]], %[[PRIV_REF:.*]]: ![[X_TYPE]]): +! CHECK: %[[ORIG_VAL:.*]] = fir.load %[[ORIG_PTR]] : !fir.ptr +! CHECK: hlfir.assign %[[ORIG_VAL]] to %[[PRIV_REF]] temporary_lhs : f32, ![[X_TYPE]] +! CHECK: omp.yield(%[[PRIV_REF]] : ![[X_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/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 From fe965f615dd274552d5f14160f9d425e70880955 Mon Sep 17 00:00:00 2001 From: ergawy Date: Wed, 31 Jul 2024 07:38:18 -0500 Subject: [PATCH 2/3] handle review comments --- flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 5 ++--- mlir/test/Target/LLVMIR/openmp-private.mlir | 3 ++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp index 71c6d1ecb797e..d785d24284352 100644 --- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp +++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp @@ -557,9 +557,8 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym, : firOpBuilder.createConvert( cloneAddr.getLoc(), symType, cloneAddr); - auto yieldOp = firOpBuilder.create( - hsb.getAddr().getLoc(), yieldedValue); - yieldOp.getOperand(0).getType(); + firOpBuilder.create(hsb.getAddr().getLoc(), + yieldedValue); symTable->popScope(); } diff --git a/mlir/test/Target/LLVMIR/openmp-private.mlir b/mlir/test/Target/LLVMIR/openmp-private.mlir index 6bc73fc6f102b..e76f7e4d40f7a 100644 --- a/mlir/test/Target/LLVMIR/openmp-private.mlir +++ b/mlir/test/Target/LLVMIR/openmp-private.mlir @@ -263,9 +263,10 @@ omp.private {type = firstprivate} @_QFequivalenceEx_firstprivate_ptr_f32 : !llvm } // CHECK: define internal void @_QPequivalence..omp_par +// CHECK-NOT: define {{.*}} @{{.*}} // CHECK: %[[PRIV_ALLOC:.*]] = alloca float, i64 1, align 4 // CHECK: %[[HOST_VAL:.*]] = load float, ptr %{{.*}}, align 4 -// Test that we initialzie the firstprivate variable. +// Test that we initialize 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 From 174abf68c437d8a7838c555c483da3fc0ac1aea9 Mon Sep 17 00:00:00 2001 From: ergawy Date: Thu, 1 Aug 2024 04:31:00 -0500 Subject: [PATCH 3/3] add comment --- flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp index d785d24284352..e1a193edc004a 100644 --- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp +++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp @@ -552,6 +552,10 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym, mlir::Value cloneAddr = symTable->shallowLookupSymbol(*sym).getAddr(); mlir::Type cloneType = cloneAddr.getType(); + // A `convert` op is required for 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`, + // therefore we convert to proper symbol type. mlir::Value yieldedValue = (symType == cloneType) ? cloneAddr : firOpBuilder.createConvert(