Skip to content

Conversation

@skatrak
Copy link
Member

@skatrak skatrak commented Mar 11, 2025

This patch simplifies the definition of ClauseProcessor::processMapObjects by hoisting the creation of the MLIR symbol associated to an existing omp.declare_mapper operation outside of the loop processing all mapped objects.

That change removes some inter-iteration dependencies that made the implementation more difficult to follow.

This patch simplifies the definition of `ClauseProcessor::processMapObjects` by
hoisting the creation of the MLIR symbol associated to an existing
`omp.declare_mapper` operation outside of the loop processing all mapped
objects.

That change removes some inter-iteration dependencies that made the
implementation more difficult to follow.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Mar 11, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 11, 2025

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

Author: Sergio Afonso (skatrak)

Changes

This patch simplifies the definition of ClauseProcessor::processMapObjects by hoisting the creation of the MLIR symbol associated to an existing omp.declare_mapper operation outside of the loop processing all mapped objects.

That change removes some inter-iteration dependencies that made the implementation more difficult to follow.


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

1 Files Affected:

  • (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+17-15)
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index dab3182e0162f..dda2ac76df3b2 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -928,8 +928,24 @@ void ClauseProcessor::processMapObjects(
     llvm::SmallVectorImpl<const semantics::Symbol *> &mapSyms,
     llvm::StringRef mapperIdNameRef) const {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+
+  // Create the mapper symbol from its name, if specified.
   mlir::FlatSymbolRefAttr mapperId;
-  std::string mapperIdName = mapperIdNameRef.str();
+  if (!mapperIdNameRef.empty() && !objects.empty()) {
+    std::string mapperIdName = mapperIdNameRef.str();
+    if (mapperIdName == "default") {
+      const omp::Object &object = objects.front();
+      auto &typeSpec = object.sym()->owner().IsDerivedType()
+                           ? *object.sym()->owner().derivedTypeSpec()
+                           : object.sym()->GetType()->derivedTypeSpec();
+      mapperIdName = typeSpec.name().ToString() + ".default";
+      mapperIdName = converter.mangleName(mapperIdName, *typeSpec.GetScope());
+    }
+    assert(converter.getModuleOp().lookupSymbol(mapperIdName) &&
+           "mapper not found");
+    mapperId =
+        mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(), mapperIdName);
+  }
 
   for (const omp::Object &object : objects) {
     llvm::SmallVector<mlir::Value> bounds;
@@ -962,20 +978,6 @@ void ClauseProcessor::processMapObjects(
       }
     }
 
-    if (!mapperIdName.empty()) {
-      if (mapperIdName == "default") {
-        auto &typeSpec = object.sym()->owner().IsDerivedType()
-                             ? *object.sym()->owner().derivedTypeSpec()
-                             : object.sym()->GetType()->derivedTypeSpec();
-        mapperIdName = typeSpec.name().ToString() + ".default";
-        mapperIdName = converter.mangleName(mapperIdName, *typeSpec.GetScope());
-      }
-      assert(converter.getModuleOp().lookupSymbol(mapperIdName) &&
-             "mapper not found");
-      mapperId = mlir::FlatSymbolRefAttr::get(&converter.getMLIRContext(),
-                                              mapperIdName);
-      mapperIdName.clear();
-    }
     // Explicit map captures are captured ByRef by default,
     // optimisation passes may alter this to ByCopy or other capture
     // types to optimise

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Member

@TIFitis TIFitis left a comment

Choose a reason for hiding this comment

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

LG

@skatrak skatrak merged commit cf68c93 into llvm:main Mar 12, 2025
14 checks passed
@skatrak skatrak deleted the declare-mapper-sym-nfc branch March 12, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flang:fir-hlfir flang Flang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants