Fix semantic check for default declare mappers #139593
Conversation
TIFitis
requested review from
kiranchandramohan,
Leporacanthicus,
raghavendhra and
Copilot
llvmbot
added
flang
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the semantic checking for default declare mappers so that each derived type is allowed one default mapper rather than one across all derived types.
- Updated test cases in declare-mapper03.f90 to reflect the new behavior.
- Revised expected output in declare-mapper-symbols.f90 to include the derived type name prefix for default mappers.
- Refactored the default mapper symbol creation in resolve-names.cpp to generate a default name based on the derived type.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| flang/test/Semantics/OpenMP/declare-mapper03.f90 | Removed redundant type t2 and updated mapper mapping in the test for default. |
| flang/test/Semantics/OpenMP/declare-mapper-symbols.f90 | Adjusted check statements to expect the derived type name in default mapper names. |
| flang/lib/Semantics/resolve-names.cpp | Refactored default symbol creation logic using a static vector for default names. |
| &MakeSymbol(*mapperName, MiscDetails{MiscDetails::Kind::ConstructName}); | ||
| } else { | ||
| const auto &type = std::get<parser::TypeSpec>(spec.t); | ||
| static llvm::SmallVector<std::string> defaultNames; |
There was a problem hiding this comment.
[nitpick] Using a static llvm::SmallVector for defaultNames may result in unexpected persistent state across invocations. Consider using a local container or clearing the vector at the beginning of the function to ensure that state does not accumulate unintentionally.
| static llvm::SmallVector<std::string> defaultNames; | |
| llvm::SmallVector<std::string> defaultNames; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I can see this is intentionally static to make sure that these runtime-created strings don't get destructed while symbols are still in use, since CharBlock doesn't own the data, but it doesn't seem to me like a very clean approach.
Not sure if perhaps adding some generic static storage for situations like this to the semantics::Scope class, something looking similar to the allSymbols field, would be a better idea or if there currently are any facilities to store strings not present in the original source to be used as symbols.
CC: @kiranchandramohan, @tblah.
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-semantics Author: Akash Banerjee (TIFitis) ChangesThe current semantic check in place is incorrect, this patch fixes this. Up to 1 'default' named mapper is allowed for each derived type. Co-authored-by: Raghu Maddhipatla <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/139593.diff 3 Files Affected:
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index b2979690f78e7..1fd0ea007319d 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -38,6 +38,7 @@
#include "flang/Semantics/type.h"
#include "flang/Support/Fortran.h"
#include "flang/Support/default-kinds.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/raw_ostream.h"
#include <list>
#include <map>
@@ -1766,14 +1767,6 @@ void OmpVisitor::ProcessMapperSpecifier(const parser::OmpMapperSpecifier &spec,
// just following the natural flow, the map clauses gets processed before
// the type has been fully processed.
BeginDeclTypeSpec();
- if (auto &mapperName{std::get<std::optional<parser::Name>>(spec.t)}) {
- mapperName->symbol =
- &MakeSymbol(*mapperName, MiscDetails{MiscDetails::Kind::ConstructName});
- } else {
- const parser::CharBlock defaultName{"default", 7};
- MakeSymbol(
- defaultName, Attrs{}, MiscDetails{MiscDetails::Kind::ConstructName});
- }
PushScope(Scope::Kind::OtherConstruct, nullptr);
Walk(std::get<parser::TypeSpec>(spec.t));
@@ -1783,6 +1776,18 @@ void OmpVisitor::ProcessMapperSpecifier(const parser::OmpMapperSpecifier &spec,
Walk(clauses);
EndDeclTypeSpec();
PopScope();
+
+ if (auto &mapperName{std::get<std::optional<parser::Name>>(spec.t)}) {
+ mapperName->symbol =
+ &MakeSymbol(*mapperName, MiscDetails{MiscDetails::Kind::ConstructName});
+ } else {
+ const auto &type = std::get<parser::TypeSpec>(spec.t);
+ static llvm::SmallVector<std::string> defaultNames;
+ defaultNames.emplace_back(
+ type.declTypeSpec->derivedTypeSpec().name().ToString() + ".default");
+ MakeSymbol(defaultNames.back(), Attrs{},
+ MiscDetails{MiscDetails::Kind::ConstructName});
+ }
}
void OmpVisitor::ProcessReductionSpecifier(
diff --git a/flang/test/Semantics/OpenMP/declare-mapper-symbols.f90 b/flang/test/Semantics/OpenMP/declare-mapper-symbols.f90
index b4e03bd1632e5..0dda5b4456987 100644
--- a/flang/test/Semantics/OpenMP/declare-mapper-symbols.f90
+++ b/flang/test/Semantics/OpenMP/declare-mapper-symbols.f90
@@ -2,23 +2,23 @@
program main
!CHECK-LABEL: MainProgram scope: main
- implicit none
+ implicit none
- type ty
- integer :: x
- end type ty
- !$omp declare mapper(mymapper : ty :: mapped) map(mapped, mapped%x)
- !$omp declare mapper(ty :: maptwo) map(maptwo, maptwo%x)
+ type ty
+ integer :: x
+ end type ty
+ !$omp declare mapper(mymapper : ty :: mapped) map(mapped, mapped%x)
+ !$omp declare mapper(ty :: maptwo) map(maptwo, maptwo%x)
!! Note, symbols come out in their respective scope, but not in declaration order.
-!CHECK: default: Misc ConstructName
!CHECK: mymapper: Misc ConstructName
!CHECK: ty: DerivedType components: x
+!CHECK: ty.default: Misc ConstructName
!CHECK: DerivedType scope: ty
!CHECK: OtherConstruct scope:
!CHECK: mapped (OmpMapToFrom) {{.*}} ObjectEntity type: TYPE(ty)
-!CHECK: OtherConstruct scope:
+!CHECK: OtherConstruct scope:
!CHECK: maptwo (OmpMapToFrom) {{.*}} ObjectEntity type: TYPE(ty)
-
+
end program main
diff --git a/flang/test/Semantics/OpenMP/declare-mapper03.f90 b/flang/test/Semantics/OpenMP/declare-mapper03.f90
index b70b8a67f33e0..84fc3efafb3ad 100644
--- a/flang/test/Semantics/OpenMP/declare-mapper03.f90
+++ b/flang/test/Semantics/OpenMP/declare-mapper03.f90
@@ -5,12 +5,8 @@
integer :: y
end type t1
-type :: t2
- real :: y, z
-end type t2
-
!error: 'default' is already declared in this scoping unit
!$omp declare mapper(t1::x) map(x, x%y)
-!$omp declare mapper(t2::w) map(w, w%y, w%z)
+!$omp declare mapper(t1::x) map(x)
end
|
| &MakeSymbol(*mapperName, MiscDetails{MiscDetails::Kind::ConstructName}); | ||
| } else { | ||
| const auto &type = std::get<parser::TypeSpec>(spec.t); | ||
| static llvm::SmallVector<std::string> defaultNames; |
There was a problem hiding this comment.
I can see this is intentionally static to make sure that these runtime-created strings don't get destructed while symbols are still in use, since CharBlock doesn't own the data, but it doesn't seem to me like a very clean approach.
Not sure if perhaps adding some generic static storage for situations like this to the semantics::Scope class, something looking similar to the allSymbols field, would be a better idea or if there currently are any facilities to store strings not present in the original source to be used as symbols.
CC: @kiranchandramohan, @tblah.
| MakeSymbol(defaultNames.back(), Attrs{}, | ||
| MiscDetails{MiscDetails::Kind::ConstructName}); |
There was a problem hiding this comment.
Nit: There's already an attribute-less overload of that function.
| MakeSymbol(defaultNames.back(), Attrs{}, | |
| MiscDetails{MiscDetails::Kind::ConstructName}); | |
| MakeSymbol(defaultNames.back(), MiscDetails{MiscDetails::Kind::ConstructName}); |
| const auto &type = std::get<parser::TypeSpec>(spec.t); | ||
| static llvm::SmallVector<std::string> defaultNames; | ||
| defaultNames.emplace_back( | ||
| type.declTypeSpec->derivedTypeSpec().name().ToString() + ".default"); |
There was a problem hiding this comment.
Nit: Maybe call it <x>.omp.default.mapper, to avoid any confusion later on.
The current semantic check in place is incorrect, this patch fixes this.
Up to 1 'default' named mapper should be allowed for each derived type.
The current semantic check only allows up to 1 'default' named mapper across all derived types.
Co-authored-by: Raghu Maddhipatla [email protected]