Skip to content

[OpenMP][Flang] Fix semantic check and scoping for declare mappers #139593

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

Closed
TIFitis wants to merge 3 commits into from
Closed

[OpenMP][Flang] Fix semantic check and scoping for declare mappers #139593

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a83bd68
Fix semantic check for default declare mappers.
TIFitis May 12, 2025
9e1bb4e
Merge branch 'main' into mapper_semantic
TIFitis May 14, 2025
19c5f56
Change mapper name field from parser::Name to std::string.
TIFitis May 14, 2025
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
21 changes: 13 additions & 8 deletions flang/lib/Semantics/resolve-names.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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));
Expand All @@ -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;
Copy link

Copilot AI May 12, 2025

Choose a reason for hiding this comment

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

[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.

Suggested change
static llvm::SmallVector<std::string> defaultNames;
llvm::SmallVector<std::string> defaultNames;

Copilot uses AI. Check for mistakes.
Copy link
Member

@skatrak skatrak May 13, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@tblah tblah May 14, 2025

Choose a reason for hiding this comment

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

Surely some string storage for symbol names already exists? If not then I agree that it would be better to have a utility for other situations like this. Even just wrapping the static vector in a well documented and named utility function would make it clearer what this is for.

Copy link
Member Author

@TIFitis TIFitis May 15, 2025

Choose a reason for hiding this comment

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

Hi, I encountered that even with this minor fix, some separate scoping related semantic issues would have still remained. To fix these issues altogether, I have instead taken a different approach to solving the problem.

I have changed the optional field for the mapper name to a compulsory std::string field. The parser has been updated to create a default name - "<typeSpec>.omp.default.mapper" whenever the default name is used.

Also added a new test to flang/test/Lower/OpenMP/declare-mapper.f90 for ensuring declare mapper scoping rules apply cleanly.

defaultNames.emplace_back(
type.declTypeSpec->derivedTypeSpec().name().ToString() + ".default");
MakeSymbol(defaultNames.back(), Attrs{},
MiscDetails{MiscDetails::Kind::ConstructName});
Copy link
Member

@skatrak skatrak May 13, 2025

Choose a reason for hiding this comment

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

Nit: There's already an attribute-less overload of that function.

Suggested change
MakeSymbol(defaultNames.back(), Attrs{},
MiscDetails{MiscDetails::Kind::ConstructName});
MakeSymbol(defaultNames.back(), MiscDetails{MiscDetails::Kind::ConstructName});

Copy link
Member Author

@TIFitis TIFitis May 15, 2025

Choose a reason for hiding this comment

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

Unfortunately, the attribute-less overload uses a different type for the name parameter and thus can't be used directly. Passing a default Attrs{} seems likely a cleaner solution than changing the name to the correct type so I have went with this approach for now.

}
}

void OmpVisitor::ProcessReductionSpecifier(
Expand Down
18 changes: 9 additions & 9 deletions flang/test/Semantics/OpenMP/declare-mapper-symbols.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

6 changes: 1 addition & 5 deletions flang/test/Semantics/OpenMP/declare-mapper03.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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