-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[flang][OpenMP] Don't privatize implicit symbols declare by nested BLOCK
s
#152973
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] Don't privatize implicit symbols declare by nested BLOCK
s
#152973
Conversation
…LOCK`s Fixes a bug when a block variable is marked as implicit 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.
@llvm/pr-subscribers-flang-fir-hlfir Author: Kareem Ergawy (ergawy) ChangesFixes a bug when a block variable is marked as implicit 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. Full diff: https://github.com/llvm/llvm-project/pull/152973.diff 3 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 49d8b46beb0eb..e3f792ee296f7 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -30,6 +30,7 @@
#include "flang/Semantics/tools.h"
#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/SmallSet.h"
+#include <variant>
namespace Fortran {
namespace lower {
@@ -44,6 +45,13 @@ bool DataSharingProcessor::OMPConstructSymbolVisitor::isSymbolDefineBy(
[](const auto &functionParserNode) { return false; }});
}
+bool DataSharingProcessor::OMPConstructSymbolVisitor::
+ isSymbolDefineByNestedDeclaration(const semantics::Symbol *symbol) const {
+ return symDefMap.count(symbol) &&
+ std::holds_alternative<const parser::DeclarationConstruct *>(
+ symDefMap.at(symbol));
+}
+
static bool isConstructWithTopLevelTarget(lower::pft::Evaluation &eval) {
const auto *ompEval = eval.getIf<parser::OpenMPConstruct>();
if (ompEval) {
@@ -550,17 +558,20 @@ void DataSharingProcessor::collectSymbols(
return false;
}
- return sym->test(semantics::Symbol::Flag::OmpImplicit);
- }
-
- 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
+ // Collect implicit symbols only if they are not defined by a nested
+ // `DeclarationConstruct`. 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 privatize it within the OpenMP construct.
- return visitor.isSymbolDefineBy(sym, eval) &&
+ return !visitor.isSymbolDefineByNestedDeclaration(sym) &&
+ sym->test(semantics::Symbol::Flag::OmpImplicit);
+ }
+
+ if (collectPreDetermined) {
+ // Similar to implicit symbols, collect pre-determined symbols only if
+ // they are not defined by a nested `DeclarationConstruct`
+ return !visitor.isSymbolDefineByNestedDeclaration(sym) &&
sym->test(semantics::Symbol::Flag::OmpPreDetermined);
}
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index 335699577ea84..96c9240017f00 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -77,6 +77,11 @@ class DataSharingProcessor {
bool isSymbolDefineBy(const semantics::Symbol *symbol,
lower::pft::Evaluation &eval) const;
+ // Given a \p symbol, returns true if it is defined by a nested
+ // `DeclarationConstruct`.
+ bool
+ isSymbolDefineByNestedDeclaration(const semantics::Symbol *symbol) const;
+
private:
using ConstructPtr = std::variant<const parser::OpenMPConstruct *,
const parser::DeclarationConstruct *>;
diff --git a/flang/test/Lower/OpenMP/block_implicit_privatization.f90 b/flang/test/Lower/OpenMP/block_implicit_privatization.f90
new file mode 100644
index 0000000000000..69956dac3ec70
--- /dev/null
+++ b/flang/test/Lower/OpenMP/block_implicit_privatization.f90
@@ -0,0 +1,31 @@
+! Fixes a bug when a block variable is marked as implicit 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_implicit_privatization
+ implicit none
+ integer :: i
+
+ !$omp task
+ do i=1,10
+ block
+ integer :: j
+ j = 0
+ end block
+ end do
+ !$omp end task
+end subroutine
+
+! CHECK-LABEL: func.func @_QPblock_implicit_privatization() {
+! CHECK: %[[I_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Ei"}
+! CHECK: omp.task 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 `task` loop).
+! CHECK: fir.alloca i32 {bindc_name = "j", {{.*}}}
+! CHECK: }
+! CHECK: }
+! CHECK: }
|
@llvm/pr-subscribers-flang-openmp Author: Kareem Ergawy (ergawy) ChangesFixes a bug when a block variable is marked as implicit 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. Full diff: https://github.com/llvm/llvm-project/pull/152973.diff 3 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 49d8b46beb0eb..e3f792ee296f7 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -30,6 +30,7 @@
#include "flang/Semantics/tools.h"
#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/SmallSet.h"
+#include <variant>
namespace Fortran {
namespace lower {
@@ -44,6 +45,13 @@ bool DataSharingProcessor::OMPConstructSymbolVisitor::isSymbolDefineBy(
[](const auto &functionParserNode) { return false; }});
}
+bool DataSharingProcessor::OMPConstructSymbolVisitor::
+ isSymbolDefineByNestedDeclaration(const semantics::Symbol *symbol) const {
+ return symDefMap.count(symbol) &&
+ std::holds_alternative<const parser::DeclarationConstruct *>(
+ symDefMap.at(symbol));
+}
+
static bool isConstructWithTopLevelTarget(lower::pft::Evaluation &eval) {
const auto *ompEval = eval.getIf<parser::OpenMPConstruct>();
if (ompEval) {
@@ -550,17 +558,20 @@ void DataSharingProcessor::collectSymbols(
return false;
}
- return sym->test(semantics::Symbol::Flag::OmpImplicit);
- }
-
- 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
+ // Collect implicit symbols only if they are not defined by a nested
+ // `DeclarationConstruct`. 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 privatize it within the OpenMP construct.
- return visitor.isSymbolDefineBy(sym, eval) &&
+ return !visitor.isSymbolDefineByNestedDeclaration(sym) &&
+ sym->test(semantics::Symbol::Flag::OmpImplicit);
+ }
+
+ if (collectPreDetermined) {
+ // Similar to implicit symbols, collect pre-determined symbols only if
+ // they are not defined by a nested `DeclarationConstruct`
+ return !visitor.isSymbolDefineByNestedDeclaration(sym) &&
sym->test(semantics::Symbol::Flag::OmpPreDetermined);
}
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index 335699577ea84..96c9240017f00 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -77,6 +77,11 @@ class DataSharingProcessor {
bool isSymbolDefineBy(const semantics::Symbol *symbol,
lower::pft::Evaluation &eval) const;
+ // Given a \p symbol, returns true if it is defined by a nested
+ // `DeclarationConstruct`.
+ bool
+ isSymbolDefineByNestedDeclaration(const semantics::Symbol *symbol) const;
+
private:
using ConstructPtr = std::variant<const parser::OpenMPConstruct *,
const parser::DeclarationConstruct *>;
diff --git a/flang/test/Lower/OpenMP/block_implicit_privatization.f90 b/flang/test/Lower/OpenMP/block_implicit_privatization.f90
new file mode 100644
index 0000000000000..69956dac3ec70
--- /dev/null
+++ b/flang/test/Lower/OpenMP/block_implicit_privatization.f90
@@ -0,0 +1,31 @@
+! Fixes a bug when a block variable is marked as implicit 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_implicit_privatization
+ implicit none
+ integer :: i
+
+ !$omp task
+ do i=1,10
+ block
+ integer :: j
+ j = 0
+ end block
+ end do
+ !$omp end task
+end subroutine
+
+! CHECK-LABEL: func.func @_QPblock_implicit_privatization() {
+! CHECK: %[[I_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Ei"}
+! CHECK: omp.task 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 `task` loop).
+! CHECK: fir.alloca i32 {bindc_name = "j", {{.*}}}
+! CHECK: }
+! CHECK: }
+! CHECK: }
|
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
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.
Thank you Kareem, I have just one question, otherwise this LGTM as well.
! Fixes a bug when a block variable is marked as implicit 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. |
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.
Nit: For this comment, I suggest rewording slightly to something like "When a block variable is marked as implicit private, we can simply ignore privatizing...". Otherwise, it sounds like a unit test is somehow fixing an issue.
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.
Done.
return !visitor.isSymbolDefineByNestedDeclaration(sym) && | ||
sym->test(semantics::Symbol::Flag::OmpPreDetermined); |
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.
Maybe it's because I'm not very familiar with this logic, but wouldn't the visitor.isSymbolDefineBy(sym, eval)
check need to be preserved? E.g.: visitor.isSymbolDefineBy(sym, eval) && !visitor.isSymbolDefineByNestedDeclaration(sym) && sym->test(semantics::Symbol::Flag::OmpPreDetermined)
.
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.
The symbol and its defining construct are added only once to symDefMap
. So, if we have a block
and within that block
a DeclarationConstruct
, the symbol will be paired with the DeclarationConstruct
. If the symbol is later referenced by the eval
for which we are doing the privatization, symDefMap
will not be updated again.
So the map track the first time we encounter a symbol and the context/construct where we did so.
Therefore, I think, isSymbolDefineByNestedDeclaration
is both more accurate and sufficient as a check.
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, thanks.
Fixes a bug when a block variable is marked as implicit 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.