Skip to content
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.

Sign up for GitHub

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][semantics][OpenMP] no privatisation of stmt functions #106550

Merged
merged 5 commits into from
Oct 4, 2024
Merged

[flang][semantics][OpenMP] no privatisation of stmt functions #106550

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c6b70f
[flang][semantics][OpenMP] no privatisation of stmt functions
tblah Aug 29, 2024
9a43a1d
Don't capitalise 'statement function'
tblah Aug 29, 2024
d3aa6da
Fix test
tblah Sep 2, 2024
413e71c
Ensure that statement functions are not implicitly privatised
tblah Oct 3, 2024
99572e6
Fix spelling and remove unused flag
tblah Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,15 +350,6 @@ void DataSharingProcessor::collectSymbols(
/*collectSymbols=*/true,
/*collectHostAssociatedSymbols=*/true);

// Add implicitly referenced symbols from statement functions.
if (curScope) {
for (const auto &sym : curScope->GetSymbols()) {
if (sym->test(semantics::Symbol::Flag::OmpFromStmtFunction) &&
sym->test(flag))
allSymbols.insert(&*sym);
}
}

llvm::SetVector<const semantics::Symbol *> symbolsInNestedRegions;
collectSymbolsInNestedRegions(eval, flag, symbolsInNestedRegions);

Expand All @@ -374,7 +365,8 @@ void DataSharingProcessor::collectSymbols(
return !semantics::IsProcedure(sym) &&
!sym.GetUltimate().has<semantics::DerivedTypeDetails>() &&
!sym.GetUltimate().has<semantics::NamelistDetails>() &&
!semantics::IsImpliedDoIndex(sym.GetUltimate());
!semantics::IsImpliedDoIndex(sym.GetUltimate()) &&
!semantics::IsStmtFunction(sym);
};

auto shouldCollectSymbol = [&](const semantics::Symbol *sym) {
Expand Down
29 changes: 12 additions & 17 deletions flang/lib/Semantics/resolve-directives.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
void CheckDataCopyingClause(
const parser::Name &, const Symbol &, Symbol::Flag);
void CheckAssocLoopLevel(std::int64_t level, const parser::OmpClause *clause);
void CheckObjectInNamelistOrAssociate(
void CheckObjectIsPrivatisable(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that sometimes privatisable (British English) is used and other times it's privatizable.
It would be nice to standardize one of the two in the code.
I suggest the latter, because that's how it's written in the OpenMP standard, that uses words such as privatization and privatized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should use the American spelling.

const parser::Name &, const Symbol &, Symbol::Flag);
void CheckSourceLabel(const parser::Label &);
void CheckLabelContext(const parser::CharBlock, const parser::CharBlock,
Expand Down Expand Up @@ -2225,20 +2225,7 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
return;
}

if (auto *stmtFunction{symbol->detailsIf<semantics::SubprogramDetails>()};
stmtFunction && stmtFunction->stmtFunction()) {
// Each non-dummy argument from a statement function must be handled too,
// as if it was explicitly referenced.
semantics::UnorderedSymbolSet symbols{
CollectSymbols(stmtFunction->stmtFunction().value())};
for (const auto &sym : symbols) {
if (!IsStmtFunctionDummy(sym) && !IsObjectWithDSA(*sym)) {
CreateImplicitSymbols(&*sym, Symbol::Flag::OmpFromStmtFunction);
}
}
} else {
CreateImplicitSymbols(symbol);
}
CreateImplicitSymbols(symbol);
} // within OpenMP construct
}

Expand Down Expand Up @@ -2357,7 +2344,7 @@ void OmpAttributeVisitor::ResolveOmpObject(
CheckMultipleAppearances(*name, *symbol, ompFlag);
}
if (privateDataSharingAttributeFlags.test(ompFlag)) {
CheckObjectInNamelistOrAssociate(*name, *symbol, ompFlag);
CheckObjectIsPrivatisable(*name, *symbol, ompFlag);
}

if (ompFlag == Symbol::Flag::OmpAllocate) {
Expand Down Expand Up @@ -2729,7 +2716,7 @@ void OmpAttributeVisitor::CheckDataCopyingClause(
}
}

void OmpAttributeVisitor::CheckObjectInNamelistOrAssociate(
void OmpAttributeVisitor::CheckObjectIsPrivatisable(
const parser::Name &name, const Symbol &symbol, Symbol::Flag ompFlag) {
const auto &ultimateSymbol{symbol.GetUltimate()};
llvm::StringRef clauseName{"PRIVATE"};
Expand All @@ -2750,6 +2737,14 @@ void OmpAttributeVisitor::CheckObjectInNamelistOrAssociate(
"Variable '%s' in ASSOCIATE cannot be in a %s clause"_err_en_US,
name.ToString(), clauseName.str());
}

if (stmtFunctionExprSymbols_.find(ultimateSymbol) !=
stmtFunctionExprSymbols_.end()) {
context_.Say(name.source,
"Variable '%s' in statement function expression cannot be in a "
"%s clause"_err_en_US,
name.ToString(), clauseName.str());
}
Comment on lines +2741 to +2747
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to change the name of this member function, to something like CheckObjectIsPrivatizable, now that it also checks for statement functions.

}

void OmpAttributeVisitor::CheckSourceLabel(const parser::Label &label) {
Expand Down
2 changes: 1 addition & 1 deletion flang/lib/Semantics/semantics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,8 +642,8 @@ bool Semantics::Perform() {
CanonicalizeAcc(context_.messages(), program_) &&
CanonicalizeOmp(context_.messages(), program_) &&
CanonicalizeCUDA(program_) &&
CanonicalizeDirectives(context_.messages(), program_) &&
PerformStatementSemantics(context_, program_) &&
CanonicalizeDirectives(context_.messages(), program_) &&
ModFileWriter{context_}
.set_hermeticModuleFileOutput(hermeticModuleFileOutput_)
.WriteAll();
Expand Down
10 changes: 3 additions & 7 deletions flang/test/Lower/OpenMP/statement-function.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
! Test privatization within OpenMP constructs containing statement functions.
! Test statement functions are not privatised
! RUN: %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s

!CHECK-LABEL: func @_QPtest_implicit_use
!CHECK: %[[IEXP:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFtest_implicit_useEiexp"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
!CHECK: %[[IIMP:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFtest_implicit_useEiimp"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
!CHECK: omp.parallel private({{.*firstprivate.*}} %[[IEXP]]#0 -> %[[PRIV_IEXP:[^,]+]],
!CHECK-SAME: {{.*firstprivate.*}} %[[IIMP]]#0 -> %[[PRIV_IIMP:.*]] : !fir.ref<i32>, !fir.ref<i32>)
!CHECK: omp.parallel private({{.*firstprivate.*}} %[[IEXP]]#0 -> %[[PRIV_IEXP:[^,]+]] : !fir.ref<i32>) {
!CHECK: %{{.*}}:2 = hlfir.declare %[[PRIV_IEXP]] {uniq_name = "_QFtest_implicit_useEiexp"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
!CHECK: %{{.*}}:2 = hlfir.declare %[[PRIV_IIMP]] {uniq_name = "_QFtest_implicit_useEiimp"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
subroutine test_implicit_use()
implicit none
integer :: iexp, iimp
Expand All @@ -27,9 +25,7 @@ subroutine test_implicit_use()
!CHECK: %[[PRIV_IEXP:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFtest_implicit_use2Eiexp"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
!CHECK: %[[TEMP0:.*]] = fir.load %[[IEXP]]#0 : !fir.ref<i32>
!CHECK: hlfir.assign %[[TEMP0]] to %[[PRIV_IEXP]]#0 : i32, !fir.ref<i32>
!CHECK: %[[PRIV_IIMP:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFtest_implicit_use2Eiimp"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
!CHECK: %[[TEMP1:.*]] = fir.load %[[IIMP]]#0 : !fir.ref<i32>
!CHECK: hlfir.assign %[[TEMP1]] to %[[PRIV_IIMP]]#0 : i32, !fir.ref<i32>
!CHECK-NOT: %[[PRIV_IIMP:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFtest_implicit_use2Eiimp"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
subroutine test_implicit_use2()
implicit none
integer :: iexp, iimp
Expand Down
39 changes: 39 additions & 0 deletions flang/test/Semantics/OpenMP/private03.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
! RUN: %python %S/../test_errors.py %s %flang -fopenmp
! OpenMP Version 4.5
! Variables that appear in expressions for statement function definitions
! may not appear in private, firstprivate or lastprivate clauses.

subroutine stmt_function(temp)

integer :: i, p, q, r
real :: c, f, s, v, t(10)
real, intent(in) :: temp

c(temp) = p * (temp - q) / r
f(temp) = q + (temp * r/p)
v(temp) = c(temp) + f(temp)/2 - s

p = 5
q = 32
r = 9

!ERROR: Variable 'p' in statement function expression cannot be in a PRIVATE clause
!$omp parallel private(p)
s = c(temp)
!$omp end parallel

!ERROR: Variable 's' in statement function expression cannot be in a FIRSTPRIVATE clause
!$omp parallel firstprivate(s)
s = s + f(temp)
!$omp end parallel

!ERROR: Variable 's' in statement function expression cannot be in a LASTPRIVATE clause
!$omp parallel do lastprivate(s, t)
do i = 1, 10
t(i) = v(temp) + i - s
end do
!$omp end parallel do

print *, t

end subroutine stmt_function
Loading