[Flang][OpenMP] Initial defaultmap(none) implementation #166715
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-semantics Author: None (agozillon) ChangesThis PR adds defaultmap(none) behaviour to Flang, where we emit a semantic error if variables within the target construct do not have an associated data attribute. Similar to the way default behaves, as described by the OpenMP specification. Full diff: https://github.com/llvm/llvm-project/pull/166715.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index ad456d89bc432..1dc613e4b5fa6 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1008,9 +1008,7 @@ getImplicitMapTypeAndKind(fir::FirOpBuilder &firOpBuilder,
mlir::omp::VariableCaptureKind::ByRef);
break;
case DefMap::ImplicitBehavior::Firstprivate:
- case DefMap::ImplicitBehavior::None:
- TODO(loc, "Firstprivate and None are currently unsupported defaultmap "
- "behaviour");
+ TODO(loc, "Firstprivate currently unsupported defaultmap behaviour");
break;
case DefMap::ImplicitBehavior::From:
return std::make_pair(mapFlag |= mlir::omp::ClauseMapFlags::from,
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index deb57e005a352..7dfafe3e7dd63 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2965,6 +2965,73 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
}
}
+static bool IsOpenMPPointer(const Symbol &symbol) {
+ if (IsPointer(symbol) || IsBuiltinCPtr(symbol))
+ return true;
+ return false;
+}
+
+static bool IsOpenMPAggregate(const Symbol &symbol) {
+ if (IsAllocatable(symbol) || IsOpenMPPointer(symbol))
+ return false;
+
+ const auto *type{symbol.GetType()};
+ // OpenMP categorizes Fortran characters as aggregates.
+ if (type->category() == Fortran::semantics::DeclTypeSpec::Category::Character)
+ return true;
+
+ if (const auto *det =
+ symbol.GetUltimate()
+ .detailsIf<Fortran::semantics::ObjectEntityDetails>())
+ if (det->IsArray())
+ return true;
+
+ if (type->AsDerived())
+ return true;
+
+ if (IsDeferredShape(symbol) || IsAssumedRank(symbol) ||
+ IsAssumedShape(symbol))
+ return true;
+ return false;
+}
+
+static bool IsOpenMPScalar(const Symbol &symbol) {
+ if (IsOpenMPAggregate(symbol) || IsOpenMPPointer(symbol) ||
+ IsAllocatable(symbol))
+ return false;
+ const auto *type{symbol.GetType()};
+ if ((!symbol.GetShape() || symbol.GetShape()->empty()) &&
+ (type->category() ==
+ Fortran::semantics::DeclTypeSpec::Category::Numeric ||
+ type->category() ==
+ Fortran::semantics::DeclTypeSpec::Category::Logical))
+ return true;
+ return false;
+}
+
+static bool DefaultMapCategoryMatchesSymbol(
+ parser::OmpVariableCategory::Value category, const Symbol &symbol) {
+ using varCat = parser::OmpVariableCategory::Value;
+ switch (category) {
+ case varCat::Scalar:
+ return IsOpenMPScalar(symbol);
+ break;
+ case varCat::Allocatable:
+ return IsAllocatable(symbol);
+ break;
+ case varCat::Aggregate:
+ return IsOpenMPAggregate(symbol);
+ break;
+ case varCat::Pointer:
+ return IsOpenMPPointer(symbol);
+ break;
+ case varCat::All:
+ return true;
+ break;
+ }
+ return false;
+}
+
// For OpenMP constructs, check all the data-refs within the constructs
// and adjust the symbol for each Name if necessary
void OmpAttributeVisitor::Post(const parser::Name &name) {
@@ -3000,6 +3067,36 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
}
}
+ // TODO: handle case where default and defaultmap are present on the same
+ // construct and conflict, defaultmap should supersede default if they
+ // conflict.
+ if (!GetContext().defaultMap.empty()) {
+ // Checked before implicit data sharing attributes as this rule ignores
+ // them and expects explicit predetermined/specified attributes to be in
+ // place for the types specified.
+ if (Symbol * found{currScope().FindSymbol(name.source)}) {
+ // If the variable has declare target applied to it (enter or link) it
+ // is exempt from defaultmap(none) restrictions
+ if (!symbol->GetUltimate().test(Symbol::Flag::OmpDeclareTarget)) {
+ auto &dMap = GetContext().defaultMap;
+ for (auto defaults : dMap) {
+ if (defaults.second ==
+ parser::OmpDefaultmapClause::ImplicitBehavior::None) {
+ if (DefaultMapCategoryMatchesSymbol(defaults.first, *found)) {
+ if (!IsObjectWithDSA(*symbol)) {
+ context_.Say(name.source,
+ "The DEFAULTMAP(NONE) clause requires that '%s' must be "
+ "listed in a "
+ "data-sharing attribute, data-mapping attribute, or is_device_ptr clause"_err_en_US,
+ symbol->name());
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+
if (Symbol * found{currScope().FindSymbol(name.source)}) {
if (found->GetUltimate().test(semantics::Symbol::Flag::OmpThreadprivate))
return;
diff --git a/flang/test/Lower/OpenMP/Todo/defaultmap-clause-firstprivate.f90 b/flang/test/Lower/OpenMP/Todo/defaultmap-clause-firstprivate.f90
index 6818c39f63a3c..ce36dd45e3ab9 100644
--- a/flang/test/Lower/OpenMP/Todo/defaultmap-clause-firstprivate.f90
+++ b/flang/test/Lower/OpenMP/Todo/defaultmap-clause-firstprivate.f90
@@ -6,7 +6,7 @@ subroutine f00
! NOTE: This is implemented for scalars as it is the default behaviour, so we utilise
! a different data type.
integer, allocatable :: i
- !CHECK: not yet implemented: Firstprivate and None are currently unsupported defaultmap behaviour
+ !CHECK: not yet implemented: Firstprivate currently unsupported defaultmap behaviour
!$omp target defaultmap(firstprivate)
i = 10
!$omp end target
diff --git a/flang/test/Lower/OpenMP/Todo/defaultmap-clause-none.f90 b/flang/test/Lower/OpenMP/Todo/defaultmap-clause-none.f90
deleted file mode 100644
index 287eb4a9dfe8f..0000000000000
--- a/flang/test/Lower/OpenMP/Todo/defaultmap-clause-none.f90
+++ /dev/null
@@ -1,11 +0,0 @@
-!RUN: %not_todo_cmd bbc -emit-hlfir -fopenmp -fopenmp-version=51 -o - %s 2>&1 | FileCheck %s
-!RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=51 -o - %s 2>&1 | FileCheck %s
-
-subroutine f00
- implicit none
- integer :: i
- !CHECK: not yet implemented: Firstprivate and None are currently unsupported defaultmap behaviour
- !$omp target defaultmap(none)
- i = 10
- !$omp end target
-end
diff --git a/flang/test/Semantics/OpenMP/defaultmap-clause-none.f90 b/flang/test/Semantics/OpenMP/defaultmap-clause-none.f90
new file mode 100644
index 0000000000000..08e8ebc995097
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/defaultmap-clause-none.f90
@@ -0,0 +1,96 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-version=51
+
+subroutine defaultmap_all_none_no_errors
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none) map(to: index, alloca) map(tofrom: array, ptr)
+ do index = 1, 10
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_all_none_no_errors
+
+subroutine defaultmap_all_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+ !$omp target defaultmap(none)
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'ptr' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'array' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'alloca' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_all_none
+
+subroutine defaultmap_scalar_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none: scalar)
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_scalar_none
+
+subroutine defaultmap_pointer_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none: pointer)
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'ptr' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_pointer_none
+
+subroutine defaultmap_allocatable_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none: allocatable)
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'alloca' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_allocatable_none
+
+subroutine defaultmap_aggregate_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none: aggregate)
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'array' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_aggregate_none
|
Member
|
@llvm/pr-subscribers-flang-openmp Author: None (agozillon) ChangesThis PR adds defaultmap(none) behaviour to Flang, where we emit a semantic error if variables within the target construct do not have an associated data attribute. Similar to the way default behaves, as described by the OpenMP specification. Full diff: https://github.com/llvm/llvm-project/pull/166715.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index ad456d89bc432..1dc613e4b5fa6 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1008,9 +1008,7 @@ getImplicitMapTypeAndKind(fir::FirOpBuilder &firOpBuilder,
mlir::omp::VariableCaptureKind::ByRef);
break;
case DefMap::ImplicitBehavior::Firstprivate:
- case DefMap::ImplicitBehavior::None:
- TODO(loc, "Firstprivate and None are currently unsupported defaultmap "
- "behaviour");
+ TODO(loc, "Firstprivate currently unsupported defaultmap behaviour");
break;
case DefMap::ImplicitBehavior::From:
return std::make_pair(mapFlag |= mlir::omp::ClauseMapFlags::from,
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index deb57e005a352..7dfafe3e7dd63 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2965,6 +2965,73 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
}
}
+static bool IsOpenMPPointer(const Symbol &symbol) {
+ if (IsPointer(symbol) || IsBuiltinCPtr(symbol))
+ return true;
+ return false;
+}
+
+static bool IsOpenMPAggregate(const Symbol &symbol) {
+ if (IsAllocatable(symbol) || IsOpenMPPointer(symbol))
+ return false;
+
+ const auto *type{symbol.GetType()};
+ // OpenMP categorizes Fortran characters as aggregates.
+ if (type->category() == Fortran::semantics::DeclTypeSpec::Category::Character)
+ return true;
+
+ if (const auto *det =
+ symbol.GetUltimate()
+ .detailsIf<Fortran::semantics::ObjectEntityDetails>())
+ if (det->IsArray())
+ return true;
+
+ if (type->AsDerived())
+ return true;
+
+ if (IsDeferredShape(symbol) || IsAssumedRank(symbol) ||
+ IsAssumedShape(symbol))
+ return true;
+ return false;
+}
+
+static bool IsOpenMPScalar(const Symbol &symbol) {
+ if (IsOpenMPAggregate(symbol) || IsOpenMPPointer(symbol) ||
+ IsAllocatable(symbol))
+ return false;
+ const auto *type{symbol.GetType()};
+ if ((!symbol.GetShape() || symbol.GetShape()->empty()) &&
+ (type->category() ==
+ Fortran::semantics::DeclTypeSpec::Category::Numeric ||
+ type->category() ==
+ Fortran::semantics::DeclTypeSpec::Category::Logical))
+ return true;
+ return false;
+}
+
+static bool DefaultMapCategoryMatchesSymbol(
+ parser::OmpVariableCategory::Value category, const Symbol &symbol) {
+ using varCat = parser::OmpVariableCategory::Value;
+ switch (category) {
+ case varCat::Scalar:
+ return IsOpenMPScalar(symbol);
+ break;
+ case varCat::Allocatable:
+ return IsAllocatable(symbol);
+ break;
+ case varCat::Aggregate:
+ return IsOpenMPAggregate(symbol);
+ break;
+ case varCat::Pointer:
+ return IsOpenMPPointer(symbol);
+ break;
+ case varCat::All:
+ return true;
+ break;
+ }
+ return false;
+}
+
// For OpenMP constructs, check all the data-refs within the constructs
// and adjust the symbol for each Name if necessary
void OmpAttributeVisitor::Post(const parser::Name &name) {
@@ -3000,6 +3067,36 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
}
}
+ // TODO: handle case where default and defaultmap are present on the same
+ // construct and conflict, defaultmap should supersede default if they
+ // conflict.
+ if (!GetContext().defaultMap.empty()) {
+ // Checked before implicit data sharing attributes as this rule ignores
+ // them and expects explicit predetermined/specified attributes to be in
+ // place for the types specified.
+ if (Symbol * found{currScope().FindSymbol(name.source)}) {
+ // If the variable has declare target applied to it (enter or link) it
+ // is exempt from defaultmap(none) restrictions
+ if (!symbol->GetUltimate().test(Symbol::Flag::OmpDeclareTarget)) {
+ auto &dMap = GetContext().defaultMap;
+ for (auto defaults : dMap) {
+ if (defaults.second ==
+ parser::OmpDefaultmapClause::ImplicitBehavior::None) {
+ if (DefaultMapCategoryMatchesSymbol(defaults.first, *found)) {
+ if (!IsObjectWithDSA(*symbol)) {
+ context_.Say(name.source,
+ "The DEFAULTMAP(NONE) clause requires that '%s' must be "
+ "listed in a "
+ "data-sharing attribute, data-mapping attribute, or is_device_ptr clause"_err_en_US,
+ symbol->name());
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+
if (Symbol * found{currScope().FindSymbol(name.source)}) {
if (found->GetUltimate().test(semantics::Symbol::Flag::OmpThreadprivate))
return;
diff --git a/flang/test/Lower/OpenMP/Todo/defaultmap-clause-firstprivate.f90 b/flang/test/Lower/OpenMP/Todo/defaultmap-clause-firstprivate.f90
index 6818c39f63a3c..ce36dd45e3ab9 100644
--- a/flang/test/Lower/OpenMP/Todo/defaultmap-clause-firstprivate.f90
+++ b/flang/test/Lower/OpenMP/Todo/defaultmap-clause-firstprivate.f90
@@ -6,7 +6,7 @@ subroutine f00
! NOTE: This is implemented for scalars as it is the default behaviour, so we utilise
! a different data type.
integer, allocatable :: i
- !CHECK: not yet implemented: Firstprivate and None are currently unsupported defaultmap behaviour
+ !CHECK: not yet implemented: Firstprivate currently unsupported defaultmap behaviour
!$omp target defaultmap(firstprivate)
i = 10
!$omp end target
diff --git a/flang/test/Lower/OpenMP/Todo/defaultmap-clause-none.f90 b/flang/test/Lower/OpenMP/Todo/defaultmap-clause-none.f90
deleted file mode 100644
index 287eb4a9dfe8f..0000000000000
--- a/flang/test/Lower/OpenMP/Todo/defaultmap-clause-none.f90
+++ /dev/null
@@ -1,11 +0,0 @@
-!RUN: %not_todo_cmd bbc -emit-hlfir -fopenmp -fopenmp-version=51 -o - %s 2>&1 | FileCheck %s
-!RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=51 -o - %s 2>&1 | FileCheck %s
-
-subroutine f00
- implicit none
- integer :: i
- !CHECK: not yet implemented: Firstprivate and None are currently unsupported defaultmap behaviour
- !$omp target defaultmap(none)
- i = 10
- !$omp end target
-end
diff --git a/flang/test/Semantics/OpenMP/defaultmap-clause-none.f90 b/flang/test/Semantics/OpenMP/defaultmap-clause-none.f90
new file mode 100644
index 0000000000000..08e8ebc995097
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/defaultmap-clause-none.f90
@@ -0,0 +1,96 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-version=51
+
+subroutine defaultmap_all_none_no_errors
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none) map(to: index, alloca) map(tofrom: array, ptr)
+ do index = 1, 10
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_all_none_no_errors
+
+subroutine defaultmap_all_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+ !$omp target defaultmap(none)
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'ptr' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'array' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'alloca' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_all_none
+
+subroutine defaultmap_scalar_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none: scalar)
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'index' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_scalar_none
+
+subroutine defaultmap_pointer_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none: pointer)
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'ptr' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_pointer_none
+
+subroutine defaultmap_allocatable_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none: allocatable)
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'alloca' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_allocatable_none
+
+subroutine defaultmap_aggregate_none
+ implicit none
+ real :: array(10)
+ integer, pointer :: ptr(:)
+ real, allocatable :: alloca
+ integer :: index
+
+ !$omp target defaultmap(none: aggregate)
+ do index = 1, 10
+!ERROR: The DEFAULTMAP(NONE) clause requires that 'array' must be listed in a data-sharing attribute, data-mapping attribute, or is_device_ptr clause
+ ptr(index) = array(index) + alloca
+ end do
+ !$omp end target
+end subroutine defaultmap_aggregate_none
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
agozillon
force-pushed
the
defaultmap-none-implementation
branch
2 times, most recently
from
d79670e to
2c0281a
Compare
kparzysz
reviewed
Nov 7, 2025
|
|
||
| static bool DefaultMapCategoryMatchesSymbol( | ||
| parser::OmpVariableCategory::Value category, const Symbol &symbol) { | ||
| using varCat = parser::OmpVariableCategory::Value; |
Contributor
There was a problem hiding this comment.
Style: use UpperCamel for types, i.e. VarCat.
| switch (category) { | ||
| case varCat::Scalar: | ||
| return IsOpenMPScalar(symbol); | ||
| break; |
Contributor
There was a problem hiding this comment.
Please remove the breaks after returns.
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
This whole thing above really belongs in check-omp-structure. Would it be hard to move it there?
Contributor
Author
There was a problem hiding this comment.
Not too sure, I can try to do so on Monday when I look at updating the PR! The main reason it's here is that the equivalent check for default is above, and there is some interconnection between the two clauses if they appear on the same construct that I'll need to handle in a follow up PR.
The only thing I can think of that might be a problem (aside from needing to supersede the default check above) is if check-omp-structure occurs after resolve-directives, as resolve-directives applies implicit DSAs to things almost immediately after this and from my understanding (from my admittedly shaky reading of the specification and testing with Clang at least) if you apply defaultmap(none), it'd require everything to be explicitly specified in a data sharing attribute or data mapping attribute (or use_device/has_device etc.). So the implicit application of the DSAs below would cause issues (and from last I checked they don't all get the implicit keyword applied at the moment as it impacts some other handling elsewhere, so it's likely not possible to check if all of the DSAs have appeared implicitly or if a user explicitly specified it).
Contributor
Author
There was a problem hiding this comment.
p.s. Thank you for the quick review!
Contributor
There was a problem hiding this comment.
The setting of flags on a symbol should be done here, but error reporting should go to check-omp-structure.
Contributor
Author
There was a problem hiding this comment.
Aware of that, I'd have to move the checking for default(none) as well as defaultmap(none) and we wouldn't want to set the flags on these symbols implicitly here and skip them when things are set to none, I'm not sure if there would be any fallout from here to check-omp-structure because of that, will see soon enough I suppose.
Contributor
There was a problem hiding this comment.
At the second thought, feel free to leave it here. I'll have to investigate a bit more closely how we handle DSA and mapping attributes here.
Contributor
Author
There was a problem hiding this comment.
Sounds good @kparzysz thank you very much. I'm a little sparse on knowledge for it myself, other than having worked with this segment of code a little bit when adding the implicit firstprivate DSA, from what I recollect the segment directly after this applies implicit DSA's (firstprivate etc.), but we don't mark every implicitly applied DSA with implicit, if this was fixed I imagine moving both the defaultmap/default segments to check-omp-structure would be fairly easy (if it isn't already and I'm overthinking it) as we could just check if the DSA's are implicitly generated and error out if they are. I recall there being an issue that the implicit flag was tied to some other CodeGen related things, making it not quite possible to tag everything that's implicit with it currently. Perhaps a new flag could be used in its place but that doesn't seem like the most elegant solution.
| if (type->category() == Fortran::semantics::DeclTypeSpec::Category::Character) | ||
| return true; | ||
|
|
||
| if (const auto *det = symbol.GetUltimate() |
kparzysz
reviewed
Nov 7, 2025
flang/lib/Lower/OpenMP/OpenMP.cpp
Outdated
| case DefMap::ImplicitBehavior::None: | ||
| TODO(loc, "Firstprivate and None are currently unsupported defaultmap " | ||
| "behaviour"); | ||
| TODO(loc, "Firstprivate currently unsupported defaultmap behaviour"); |
kparzysz
reviewed
Nov 10, 2025
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
The setting of flags on a symbol should be done here, but error reporting should go to check-omp-structure.
| // Checked before implicit data sharing attributes as this rule ignores | ||
| // them and expects explicit predetermined/specified attributes to be in | ||
| // place for the types specified. | ||
| if (Symbol * found{currScope().FindSymbol(name.source)}) { |
Contributor
There was a problem hiding this comment.
Is your clang-format inserting a space after the *? Could you remove it, it just looks wrong. Clang-format does that some time when it thinks the first symbol is a variable, not a type...
Contributor
Author
There was a problem hiding this comment.
I believe it is clang-format and it seems the CI's format checker is very adamant about the space after the * unfortunately (I do agree it doesn't look right, seem like it's also done elsewhere in the file, likely due to clang-format)
agozillon
force-pushed
the
defaultmap-none-implementation
branch
from
2c0281a to
135c1c1
Compare
This PR adds defaultmap(none) behaviour to Flang, where we emit a semantic error if variables within the target construct do not have an associated data attribute. Similar to the way default behaves, as described by the OpenMP specification.
agozillon
force-pushed
the
defaultmap-none-implementation
branch
from
135c1c1 to
8a1fc2c
Compare
Contributor
Author
|
Tried to address the nits in the last commit and rebased on top of main. |
bhandarkar-pranav
approved these changes
Nov 10, 2025
Contributor
bhandarkar-pranav
left a comment
bhandarkar-pranav
left a comment
There was a problem hiding this comment.
Thank you for this PR, @agozillon. LGTM.
Contributor
Author
|
Thank you very much @kparzysz and @bhandarkar-pranav for the reviews! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds defaultmap(none) behaviour to Flang, where we emit a semantic error if variables within the target construct do not have an associated data attribute. Similar to the way default behaves, as described by the OpenMP specification.