[flang-commits] [flang] [flang][OpenMP] Don't privatize pre-determined symbols declare by nested `BLOCK`s (PR #152652)
Kareem Ergawy via flang-commits
flang-commits at lists.llvm.org
Mon Aug 11 00:21:38 PDT 2025
https://github.com/ergawy updated https://github.com/llvm/llvm-project/pull/152652
>From 1f2e69d52bb8b5d8c5ad0b91ea03db5d5528c264 Mon Sep 17 00:00:00 2001
From: ergawy <kareem.ergawy at amd.com>
Date: Fri, 8 Aug 2025 01:48:28 -0500
Subject: [PATCH 1/2] [flang][OpenMP] Don't pritvatize pre-determined symbols
declare by nested `BLOCK`s
Fixes a bug when a block variable is marked as pre-determined private. In such
case, we can simply ignore privatizing that symbol within the context of the
currrent OpenMP construct since the "private" allocation for the symbol will
be emitted within the nested block anyway.
---
.../lib/Lower/OpenMP/DataSharingProcessor.cpp | 12 +++++--
flang/lib/Lower/OpenMP/DataSharingProcessor.h | 16 +++++++---
.../block_predetermined_privatization.f90 | 32 +++++++++++++++++++
3 files changed, 54 insertions(+), 6 deletions(-)
create mode 100644 flang/test/Lower/OpenMP/block_predetermined_privatization.f90
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 67a9a4675f115..2ad2159bf806e 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -553,8 +553,16 @@ void DataSharingProcessor::collectSymbols(
return sym->test(semantics::Symbol::Flag::OmpImplicit);
}
- if (collectPreDetermined)
- return sym->test(semantics::Symbol::Flag::OmpPreDetermined);
+ if (collectPreDetermined) {
+ // Collect pre-determined symbols only if they are defined by the current
+ // OpenMP evaluation. If `sym` is not defined by the current OpenMP
+ // evaluation then it is defined by a block nested within the OpenMP
+ // construct. This, in turn, means that the private allocation for the
+ // symbol will be emitted as part of the nested block and there is no need
+ // to prvatize it within the OpenMP construct.
+ return visitor.isSymbolDefineBy(sym, eval) &&
+ sym->test(semantics::Symbol::Flag::OmpPreDetermined);
+ }
return !sym->test(semantics::Symbol::Flag::OmpImplicit) &&
!sym->test(semantics::Symbol::Flag::OmpPreDetermined);
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index 96e7fa6d5d93c..9444b698244a0 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -58,13 +58,21 @@ class DataSharingProcessor {
}
void Post(const parser::Name &name) {
- auto *current = !constructs.empty() ? constructs.back() : nullptr;
+ const void *current = !constructs.empty() ? constructs.back() : nullptr;
symDefMap.try_emplace(name.symbol, current);
}
- llvm::SmallVector<const parser::OpenMPConstruct *> constructs;
- llvm::DenseMap<semantics::Symbol *, const parser::OpenMPConstruct *>
- symDefMap;
+ bool Pre(const parser::DeclarationConstruct &decl) {
+ constructs.push_back(&decl);
+ return true;
+ }
+
+ void Post(const parser::DeclarationConstruct &decl) {
+ constructs.pop_back();
+ }
+
+ llvm::SmallVector<const void *> constructs;
+ llvm::DenseMap<semantics::Symbol *, const void *> symDefMap;
/// Given a \p symbol and an \p eval, returns true if eval is the OMP
/// construct that defines symbol.
diff --git a/flang/test/Lower/OpenMP/block_predetermined_privatization.f90 b/flang/test/Lower/OpenMP/block_predetermined_privatization.f90
new file mode 100644
index 0000000000000..12346c14ef5d9
--- /dev/null
+++ b/flang/test/Lower/OpenMP/block_predetermined_privatization.f90
@@ -0,0 +1,32 @@
+! Fixes a bug when a block variable is marked as pre-determined private. In such
+! case, we can simply ignore privatizing that symbol within the context of the
+! currrent OpenMP construct since the "private" allocation for the symbol will
+! be emitted within the nested block anyway.
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+subroutine block_predetermined_privatization
+ implicit none
+ integer :: i
+
+ !$omp parallel
+ do i=1,10
+ block
+ integer :: j
+ do j=1,10
+ end do
+ end block
+ end do
+ !$omp end parallel
+end subroutine
+
+! CHECK-LABEL: func.func @_QPblock_predetermined_privatization() {
+! CHECK: %[[I_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Ei"}
+! CHECK: omp.parallel private(@{{.*}}Ei_private_i32 %[[I_DECL]]#0 -> %{{.*}} : !fir.ref<i32>) {
+! CHECK: fir.do_loop {{.*}} {
+! Verify that `j` is allocated whithin the same scope of its block (i.e. inside
+! the `parallel` loop).
+! CHECK: fir.alloca i32 {bindc_name = "j", {{.*}}}
+! CHECK: }
+! CHECK: }
+! CHECK: }
>From 451d0369e1a35cece4007acb085d4bbc9af03ff5 Mon Sep 17 00:00:00 2001
From: ergawy <kareem.ergawy at amd.com>
Date: Mon, 11 Aug 2025 02:21:19 -0500
Subject: [PATCH 2/2] Handle review comments
---
flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 14 +++++++-------
flang/lib/Lower/OpenMP/DataSharingProcessor.h | 11 +++++++----
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 2ad2159bf806e..49d8b46beb0eb 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -36,12 +36,12 @@ namespace lower {
namespace omp {
bool DataSharingProcessor::OMPConstructSymbolVisitor::isSymbolDefineBy(
const semantics::Symbol *symbol, lower::pft::Evaluation &eval) const {
- return eval.visit(
- common::visitors{[&](const parser::OpenMPConstruct &functionParserNode) {
- return symDefMap.count(symbol) &&
- symDefMap.at(symbol) == &functionParserNode;
- },
- [](const auto &functionParserNode) { return false; }});
+ return eval.visit(common::visitors{
+ [&](const parser::OpenMPConstruct &functionParserNode) {
+ return symDefMap.count(symbol) &&
+ symDefMap.at(symbol) == ConstructPtr(&functionParserNode);
+ },
+ [](const auto &functionParserNode) { return false; }});
}
static bool isConstructWithTopLevelTarget(lower::pft::Evaluation &eval) {
@@ -559,7 +559,7 @@ void DataSharingProcessor::collectSymbols(
// evaluation then it is defined by a block nested within the OpenMP
// construct. This, in turn, means that the private allocation for the
// symbol will be emitted as part of the nested block and there is no need
- // to prvatize it within the OpenMP construct.
+ // to privatize it within the OpenMP construct.
return visitor.isSymbolDefineBy(sym, eval) &&
sym->test(semantics::Symbol::Flag::OmpPreDetermined);
}
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index 9444b698244a0..335699577ea84 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -19,6 +19,7 @@
#include "flang/Parser/parse-tree.h"
#include "flang/Semantics/symbol.h"
#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include <variant>
namespace mlir {
namespace omp {
@@ -58,7 +59,7 @@ class DataSharingProcessor {
}
void Post(const parser::Name &name) {
- const void *current = !constructs.empty() ? constructs.back() : nullptr;
+ auto current = !constructs.empty() ? constructs.back() : ConstructPtr();
symDefMap.try_emplace(name.symbol, current);
}
@@ -71,15 +72,17 @@ class DataSharingProcessor {
constructs.pop_back();
}
- llvm::SmallVector<const void *> constructs;
- llvm::DenseMap<semantics::Symbol *, const void *> symDefMap;
-
/// Given a \p symbol and an \p eval, returns true if eval is the OMP
/// construct that defines symbol.
bool isSymbolDefineBy(const semantics::Symbol *symbol,
lower::pft::Evaluation &eval) const;
private:
+ using ConstructPtr = std::variant<const parser::OpenMPConstruct *,
+ const parser::DeclarationConstruct *>;
+ llvm::SmallVector<ConstructPtr> constructs;
+ llvm::DenseMap<semantics::Symbol *, ConstructPtr> symDefMap;
+
unsigned version;
};
More information about the flang-commits
mailing list