[MLIR][OpenMP] Fix recursive mapper emission.#178453
Merged
Merged
Conversation
Recursive types can cause re-entrant mapper emission. The mapper function is created by OpenMPIRBuilder before the callbacks run, so it may already exist in the LLVM module even though it is not yet registered in the ModuleTranslation mapping table. Reuse and register it to break the recursion. Added offloading test.
Member
|
@llvm/pr-subscribers-mlir-openmp @llvm/pr-subscribers-mlir-llvm Author: Akash Banerjee (TIFitis) ChangesRecursive types can cause re-entrant mapper emission. The mapper function is created by OpenMPIRBuilder before the callbacks run, so it may already exist in the LLVM module even though it is not yet registered in the ModuleTranslation mapping table. Reuse and register it to break the recursion. Added offloading test. Full diff: https://github.com/llvm/llvm-project/pull/178453.diff 2 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 022322502a755..1955d681e4826 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -5333,6 +5333,17 @@ getOrCreateUserDefinedMapperFunc(Operation *op, llvm::IRBuilderBase &builder,
if (auto *lookupFunc = moduleTranslation.lookupFunction(mapperFuncName))
return lookupFunc;
+ // Recursive types can cause re-entrant mapper emission. The mapper function
+ // is created by OpenMPIRBuilder before the callbacks run, so it may already
+ // exist in the LLVM module even though it is not yet registered in the
+ // ModuleTranslation mapping table. Reuse and register it to break the
+ // recursion.
+ if (llvm::Function *existingFunc =
+ moduleTranslation.getLLVMModule()->getFunction(mapperFuncName)) {
+ moduleTranslation.mapFunction(mapperFuncName, existingFunc);
+ return existingFunc;
+ }
+
return emitUserDefinedMapper(declMapperOp, builder, moduleTranslation,
mapperFuncName, targetDirective);
}
@@ -5389,7 +5400,12 @@ emitUserDefinedMapper(Operation *op, llvm::IRBuilderBase &builder,
genMapInfoCB, varType, mapperFuncName, customMapperCB);
if (!newFn)
return newFn.takeError();
- moduleTranslation.mapFunction(mapperFuncName, *newFn);
+ if (llvm::Function *mappedFunc = moduleTranslation.lookupFunction(mapperFuncName)) {
+ assert(mappedFunc == *newFn &&
+ "mapper function mapping disagrees with emitted function");
+ } else {
+ moduleTranslation.mapFunction(mapperFuncName, *newFn);
+ }
return *newFn;
}
diff --git a/offload/test/offloading/fortran/recursive-default-mapper.f90 b/offload/test/offloading/fortran/recursive-default-mapper.f90
new file mode 100644
index 0000000000000..47b706d108437
--- /dev/null
+++ b/offload/test/offloading/fortran/recursive-default-mapper.f90
@@ -0,0 +1,40 @@
+! Offloading test for recursive default mapper emission
+! REQUIRES: flang, amdgpu
+
+! RUN: %libomptarget-compile-fortran-run-and-check-generic
+
+module recursive_mapper_mod
+ implicit none
+
+ type :: inner
+ integer :: value
+ type(inner), pointer :: next
+ end type inner
+
+ type :: outer
+ integer, allocatable :: arr(:)
+ type(inner), pointer :: head
+ end type outer
+
+contains
+
+end module recursive_mapper_mod
+
+program main
+ use recursive_mapper_mod
+ implicit none
+
+ type(outer) :: o
+
+ allocate(o%arr(2))
+ o%arr = [1, 2]
+
+ !$omp target map(tofrom: o)
+ o%arr(1) = o%arr(1) + 1
+ o%arr(2) = o%arr(2) + 1
+ !$omp end target
+
+ print *, o%arr(1), o%arr(2)
+end program main
+
+! CHECK: 2 3
|
Member
|
@llvm/pr-subscribers-mlir Author: Akash Banerjee (TIFitis) ChangesRecursive types can cause re-entrant mapper emission. The mapper function is created by OpenMPIRBuilder before the callbacks run, so it may already exist in the LLVM module even though it is not yet registered in the ModuleTranslation mapping table. Reuse and register it to break the recursion. Added offloading test. Full diff: https://github.com/llvm/llvm-project/pull/178453.diff 2 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 022322502a755..1955d681e4826 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -5333,6 +5333,17 @@ getOrCreateUserDefinedMapperFunc(Operation *op, llvm::IRBuilderBase &builder,
if (auto *lookupFunc = moduleTranslation.lookupFunction(mapperFuncName))
return lookupFunc;
+ // Recursive types can cause re-entrant mapper emission. The mapper function
+ // is created by OpenMPIRBuilder before the callbacks run, so it may already
+ // exist in the LLVM module even though it is not yet registered in the
+ // ModuleTranslation mapping table. Reuse and register it to break the
+ // recursion.
+ if (llvm::Function *existingFunc =
+ moduleTranslation.getLLVMModule()->getFunction(mapperFuncName)) {
+ moduleTranslation.mapFunction(mapperFuncName, existingFunc);
+ return existingFunc;
+ }
+
return emitUserDefinedMapper(declMapperOp, builder, moduleTranslation,
mapperFuncName, targetDirective);
}
@@ -5389,7 +5400,12 @@ emitUserDefinedMapper(Operation *op, llvm::IRBuilderBase &builder,
genMapInfoCB, varType, mapperFuncName, customMapperCB);
if (!newFn)
return newFn.takeError();
- moduleTranslation.mapFunction(mapperFuncName, *newFn);
+ if (llvm::Function *mappedFunc = moduleTranslation.lookupFunction(mapperFuncName)) {
+ assert(mappedFunc == *newFn &&
+ "mapper function mapping disagrees with emitted function");
+ } else {
+ moduleTranslation.mapFunction(mapperFuncName, *newFn);
+ }
return *newFn;
}
diff --git a/offload/test/offloading/fortran/recursive-default-mapper.f90 b/offload/test/offloading/fortran/recursive-default-mapper.f90
new file mode 100644
index 0000000000000..47b706d108437
--- /dev/null
+++ b/offload/test/offloading/fortran/recursive-default-mapper.f90
@@ -0,0 +1,40 @@
+! Offloading test for recursive default mapper emission
+! REQUIRES: flang, amdgpu
+
+! RUN: %libomptarget-compile-fortran-run-and-check-generic
+
+module recursive_mapper_mod
+ implicit none
+
+ type :: inner
+ integer :: value
+ type(inner), pointer :: next
+ end type inner
+
+ type :: outer
+ integer, allocatable :: arr(:)
+ type(inner), pointer :: head
+ end type outer
+
+contains
+
+end module recursive_mapper_mod
+
+program main
+ use recursive_mapper_mod
+ implicit none
+
+ type(outer) :: o
+
+ allocate(o%arr(2))
+ o%arr = [1, 2]
+
+ !$omp target map(tofrom: o)
+ o%arr(1) = o%arr(1) + 1
+ o%arr(2) = o%arr(2) + 1
+ !$omp end target
+
+ print *, o%arr(1), o%arr(2)
+end program main
+
+! CHECK: 2 3
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in MLIR's OpenMP to LLVM IR translation where recursive types cause re-entrant mapper emission, leading to crashes or incorrect behavior. The fix detects when a mapper function already exists in the LLVM module (created by OpenMPIRBuilder) but hasn't been registered yet in the ModuleTranslation mapping table, and reuses it to break the recursion.
Changes:
- Added check in
getOrCreateUserDefinedMapperFuncto detect and reuse existing mapper functions in the LLVM module - Modified
emitUserDefinedMapperto handle cases where the function is already registered during recursive emission - Added Fortran test with recursive type definition to verify the fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | Implements recursion-breaking logic for mapper function emission by checking for existing functions and conditionally registering them |
| offload/test/offloading/fortran/recursive-default-mapper.f90 | Adds test case with recursive type definition to verify correct mapper emission |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agozillon
approved these changes
Jan 28, 2026
Contributor
agozillon
left a comment
agozillon
left a comment
There was a problem hiding this comment.
LGTM! Thank you for the fix :-)
honeygoyal
pushed a commit
to honeygoyal/llvm-project
that referenced
this pull request
Jan 30, 2026
Recursive types can cause re-entrant mapper emission. The mapper function is created by OpenMPIRBuilder before the callbacks run, so it may already exist in the LLVM module even though it is not yet registered in the ModuleTranslation mapping table. Reuse and register it to break the recursion. Added offloading test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recursive types can cause re-entrant mapper emission. The mapper function is created by OpenMPIRBuilder before the callbacks run, so it may already exist in the LLVM module even though it is not yet registered in the ModuleTranslation mapping table. Reuse and register it to break the recursion. Added offloading test.