Skip to content

[NFC][OpenMP] Move the default declare mapper name suffix to OMPConstants.h #141964

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.

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

Merged
merged 2 commits into from
May 30, 2025

Conversation

TIFitis
Copy link
Member

@TIFitis TIFitis commented May 29, 2025

This patch moves the default declare mapper name suffix ".omp.default.mapper" to the OMPConstants.h file to be used everywhere for lowering.

…ants.h

This patch moves the default declare mapper name suffix ".omp.default.mapper" to the OMPConstants.h file to be used everywhere for lowering.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:parser clang:openmp OpenMP related changes to Clang labels May 29, 2025
@llvmbot
Copy link
Member

llvmbot commented May 29, 2025

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

@llvm/pr-subscribers-flang-openmp

Author: Akash Banerjee (TIFitis)

Changes

This patch moves the default declare mapper name suffix ".omp.default.mapper" to the OMPConstants.h file to be used everywhere for lowering.


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

4 Files Affected:

  • (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+1-1)
  • (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+1-1)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+1-1)
  • (modified) llvm/include/llvm/Frontend/OpenMP/OMPConstants.h (+3)
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index ebdda9885d5c2..49a3b64c03a7e 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -1148,7 +1148,7 @@ void ClauseProcessor::processMapObjects(
         typeSpec = &object.sym()->GetType()->derivedTypeSpec();
 
       if (typeSpec) {
-        mapperIdName = typeSpec->name().ToString() + ".omp.default.mapper";
+        mapperIdName = typeSpec->name().ToString() + llvm::omp::OmpDefaultMapperName;
         if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName))
           mapperIdName = converter.mangleName(mapperIdName, sym->owner());
       }
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index ddb08f74b3841..fa711060c6b90 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -2423,7 +2423,7 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
       if (sym.GetType()->category() == semantics::DeclTypeSpec::TypeDerived) {
         auto &typeSpec = sym.GetType()->derivedTypeSpec();
         std::string mapperIdName =
-            typeSpec.name().ToString() + ".omp.default.mapper";
+            typeSpec.name().ToString() + llvm::omp::OmpDefaultMapperName;
         if (auto *sym = converter.getCurrentScope().FindSymbol(mapperIdName))
           mapperIdName = converter.mangleName(mapperIdName, sym->owner());
         if (converter.getModuleOp().lookupSymbol(mapperIdName))
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index c08cd1ab80559..08326fad8c143 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1402,7 +1402,7 @@ static OmpMapperSpecifier ConstructOmpMapperSpecifier(
   // This matches the syntax: <type-spec> :: <variable-name>
   if (DerivedTypeSpec * derived{std::get_if<DerivedTypeSpec>(&typeSpec.u)}) {
     return OmpMapperSpecifier{
-        std::get<Name>(derived->t).ToString() + ".omp.default.mapper",
+        std::get<Name>(derived->t).ToString() + llvm::omp::OmpDefaultMapperName,
         std::move(typeSpec), std::move(varName)};
   }
   return OmpMapperSpecifier{std::string("omp.default.mapper"),
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h b/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
index 338b56226f204..6e1bce12af8e4 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
@@ -190,6 +190,9 @@ enum class OMPScheduleType {
   LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue */ ModifierMask)
 };
 
+// Default OpenMP mapper name suffix.
+inline constexpr const char *OmpDefaultMapperName = ".omp.default.mapper";
+
 /// Values for bit flags used to specify the mapping type for
 /// offloading.
 enum class OpenMPOffloadMappingFlags : uint64_t {

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Centralize the default OpenMP mapper name suffix by moving the literal into OMPConstants.h and updating uses across parser and lowering code.

  • Introduce OmpDefaultMapperName constant in OMPConstants.h
  • Replace hardcoded suffix in parser, lowering, and clause processor with the new constant

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h Add OmpDefaultMapperName constant for default mapper suffix
flang/lib/Parser/openmp-parsers.cpp Use llvm::omp::OmpDefaultMapperName instead of a string literal
flang/lib/Lower/OpenMP/OpenMP.cpp Replace hardcoded suffix with the new constant
flang/lib/Lower/OpenMP/ClauseProcessor.cpp Update suffix usage to reference the constant
Comments suppressed due to low confidence (2)

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:194

  • [nitpick] LLVM style guidelines recommend prefixing global constants with 'k' and naming them descriptively. Consider renaming to kOMPDefaultMapperSuffix or kDefaultOpenMPMapperSuffix to match conventions and clarify that this is a suffix.
inline constexpr const char *OmpDefaultMapperName = ".omp.default.mapper";

flang/lib/Parser/openmp-parsers.cpp:1408

  • This hardcoded literal duplicates the new constant in OMPConstants.h. Replace with std::string(llvm::omp::OmpDefaultMapperName) to keep a single source of truth and avoid divergence.
return OmpMapperSpecifier{std::string("omp.default.mapper"),

Copy link

github-actions bot commented May 29, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

LG. Thanks for the change.

@TIFitis TIFitis merged commit 99ae675 into llvm:main May 30, 2025
5 of 9 checks passed
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025
…ants.h (llvm#141964)

This patch moves the default declare mapper name suffix
".omp.default.mapper" to the OMPConstants.h file to be used everywhere
for lowering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:openmp OpenMP related changes to Clang flang:fir-hlfir flang:openmp flang:parser flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants