From c8232e33dbf79c1b347ac2000305ca56a2b45302 Mon Sep 17 00:00:00 2001 From: ergawy Date: Mon, 11 Aug 2025 03:38:59 -0500 Subject: [PATCH 1/2] [flang][OpenMP] Don't privatize implicit symbols declare by nested `BLOCK`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. --- .../lib/Lower/OpenMP/DataSharingProcessor.cpp | 25 ++++++++++----- flang/lib/Lower/OpenMP/DataSharingProcessor.h | 5 +++ .../OpenMP/block_implicit_privatization.f90 | 31 +++++++++++++++++++ 3 files changed, 54 insertions(+), 7 deletions(-) create mode 100644 flang/test/Lower/OpenMP/block_implicit_privatization.f90 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 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( + symDefMap.at(symbol)); +} + static bool isConstructWithTopLevelTarget(lower::pft::Evaluation &eval) { const auto *ompEval = eval.getIf(); 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; 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) { +! 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: } From 07dfa0ac22dc2e21aeaf269ab3e2414e39cb706d Mon Sep 17 00:00:00 2001 From: ergawy Date: Mon, 11 Aug 2025 08:20:29 -0500 Subject: [PATCH 2/2] Handle review comments --- flang/test/Lower/OpenMP/block_implicit_privatization.f90 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/flang/test/Lower/OpenMP/block_implicit_privatization.f90 b/flang/test/Lower/OpenMP/block_implicit_privatization.f90 index 69956dac3ec70..32b26ac01f181 100644 --- a/flang/test/Lower/OpenMP/block_implicit_privatization.f90 +++ b/flang/test/Lower/OpenMP/block_implicit_privatization.f90 @@ -1,7 +1,7 @@ -! 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. +! When a block variable is marked as implicit private, 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