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

TIFitis · 2025-05-12T17:48:47Z

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.

This also makes sure that declare mappers follow proper scoping rules for both default and named mappers.

Co-authored-by: Raghu Maddhipatla [email protected]

Copilot

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.

Copilot · 2025-05-12T17:49:20Z

flang/lib/Semantics/resolve-names.cpp

+        &MakeSymbol(*mapperName, MiscDetails{MiscDetails::Kind::ConstructName});
+  } else {
+    const auto &type = std::get<parser::TypeSpec>(spec.t);
+    static llvm::SmallVector<std::string> defaultNames;


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

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.

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.

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.

llvmbot · 2025-05-12T17:49:22Z

@llvm/pr-subscribers-flang-parser
@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: Akash Banerjee (TIFitis)

Changes

The current semantic check in place is incorrect, this patch fixes this.

Up to 1 'default' named mapper is 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]>

Full diff: https://github.com/llvm/llvm-project/pull/139593.diff

3 Files Affected:

(modified) flang/lib/Semantics/resolve-names.cpp (+13-8)
(modified) flang/test/Semantics/OpenMP/declare-mapper-symbols.f90 (+9-9)
(modified) flang/test/Semantics/OpenMP/declare-mapper03.f90 (+1-5)

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

skatrak

Thank you Akash! I have just one comment and minor nits.

skatrak · 2025-05-13T15:51:39Z

flang/lib/Semantics/resolve-names.cpp

+        &MakeSymbol(*mapperName, MiscDetails{MiscDetails::Kind::ConstructName});
+  } else {
+    const auto &type = std::get<parser::TypeSpec>(spec.t);
+    static llvm::SmallVector<std::string> defaultNames;


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.

skatrak · 2025-05-13T15:55:11Z

flang/lib/Semantics/resolve-names.cpp

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


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});

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.

flang/lib/Semantics/resolve-names.cpp

Copilot

Pull Request Overview

This PR fixes the semantic check for default declare mappers so that each derived type may have its own default mapper rather than a single default across all types. The key changes are a renaming of the default mapper identifier from ".default" to ".omp.default.mapper", conversion of mapper names from an optional Name to a string for clarity, and corresponding updates across tests, parsing, semantic resolution, and lowering.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
flang/test/Semantics/OpenMP/declare-mapper03.f90	Removed unused type and updated mapper declaration to match new naming
flang/test/Semantics/OpenMP/declare-mapper-symbols.f90	Updated expected symbol names for default mappers in tests
flang/test/Parser/OpenMP/metadirective-dirspec.f90	Adjusted parse-tree output from using Name to string for mapper names
flang/test/Parser/OpenMP/declare-mapper-unparse.f90	Modified unparsed mapper names from Name to string output
flang/test/Lower/OpenMP/map-mapper.f90	Updated CHECK-LABEL and CHECK lines for altered default mapper naming
flang/test/Lower/OpenMP/declare-mapper.f90	Added new test file and updated mapper name references in checks
flang/lib/Semantics/resolve-names.cpp	Changed mapper name processing to use string-based names
flang/lib/Parser/unparse.cpp	Updated mapper name extraction and printing logic for new syntax
flang/lib/Parser/openmp-parsers.cpp	Added a helper to construct an OmpMapperSpecifier using string names
flang/lib/Lower/OpenMP/OpenMP.cpp	Adjusted mapper identifier construction to use new naming convention
flang/lib/Lower/OpenMP/ClauseProcessor.cpp	Updated mapper name handling to match new default syntax
flang/include/flang/Parser/parse-tree.h	Changed the OmpMapperSpecifier tuple field from an optional Name to a string

Copilot · 2025-05-15T13:30:42Z

flang/lib/Parser/openmp-parsers.cpp

+    std::optional<Name> &&mapperName, TypeSpec &&typeSpec, Name &&varName) {
+  // If a name is present, parse: name ":" typeSpec "::" name
+  // This matches the syntax: <mapper-name> : <type-spec> :: <variable-name>
+  if (mapperName.has_value() && mapperName->ToString() != "default") {


The function ConstructOmpMapperSpecifier continues to accept a std::optional for the mapper name while the parse tree now uses a std::string. Consider aligning the interface to accept a std::string (or handle an empty string) for consistency.

Copilot · 2025-05-15T13:30:43Z

flang/include/flang/Parser/parse-tree.h

@@ -3540,7 +3540,7 @@ WRAPPER_CLASS(OmpLocatorList, std::list<OmpLocator>);
 struct OmpMapperSpecifier {
  // Absent mapper-identifier is equivalent to DEFAULT.
  TUPLE_CLASS_BOILERPLATE(OmpMapperSpecifier);
-  std::tuple<std::optional<Name>, TypeSpec, Name> t;
+  std::tuple<std::string, TypeSpec, Name> t;


[nitpick] Given the change from std::optional to std::string for mapper names, adding a comment to explain the rationale for this decision would improve clarity and help maintain consistency across the codebase.

TIFitis · 2025-05-19T15:29:40Z

Moved PR to #140560 to create a PR stack.

Fix semantic check for default declare mappers.

a83bd68

TIFitis requested review from kiranchandramohan, Leporacanthicus, raghavendhra and Copilot May 12, 2025 17:48

llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics labels May 12, 2025

Copilot AI reviewed May 12, 2025

View reviewed changes

skatrak reviewed May 13, 2025

View reviewed changes

TIFitis and others added 2 commits May 14, 2025 21:54

Merge branch 'main' into mapper_semantic

9e1bb4e

Change mapper name field from parser::Name to std::string.

19c5f56

llvmbot added flang:fir-hlfir flang:parser labels May 15, 2025

TIFitis requested a review from Copilot May 15, 2025 13:30

Copilot AI reviewed May 15, 2025

View reviewed changes

TIFitis changed the title ~~Fix semantic check for default declare mappers~~ [OpenMP][Flang] Fix semantic check for default declare mappers May 19, 2025

TIFitis changed the title ~~[OpenMP][Flang] Fix semantic check for default declare mappers~~ [OpenMP][Flang] Fix semantic check and scoping for declare mappers May 19, 2025

TIFitis closed this May 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TIFitis commented May 12, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI May 12, 2025

skatrak May 13, 2025

tblah May 14, 2025

TIFitis May 15, 2025

llvmbot commented May 12, 2025 •

edited

Loading

skatrak left a comment

skatrak May 13, 2025

skatrak May 13, 2025

TIFitis May 15, 2025

Copilot AI left a comment

Copilot AI May 15, 2025

Copilot AI May 15, 2025

TIFitis commented May 19, 2025

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

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

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

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

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

Conversation

TIFitis commented May 12, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI May 12, 2025

Choose a reason for hiding this comment

skatrak May 13, 2025

Choose a reason for hiding this comment

tblah May 14, 2025

Choose a reason for hiding this comment

TIFitis May 15, 2025

Choose a reason for hiding this comment

llvmbot commented May 12, 2025 • edited Loading

skatrak left a comment

Choose a reason for hiding this comment

skatrak May 13, 2025

Choose a reason for hiding this comment

skatrak May 13, 2025

Choose a reason for hiding this comment

TIFitis May 15, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI May 15, 2025

Choose a reason for hiding this comment

Copilot AI May 15, 2025

Choose a reason for hiding this comment

TIFitis commented May 19, 2025

TIFitis commented May 12, 2025 •

edited

Loading

llvmbot commented May 12, 2025 •

edited

Loading