[Mlir-commits] [mlir] [MLIR][OpenMP] Fix taskloop charbox privatization (PR #205568)
Tom Eccles
llvmlistbot at llvm.org
Thu Jun 25 07:30:50 PDT 2026
https://github.com/tblah updated https://github.com/llvm/llvm-project/pull/205568
>From 7433df7cf99a950456c8e4710e0d703f33416ba6 Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Wed, 24 Jun 2026 12:38:54 +0100
Subject: [PATCH 1/3] [MLIR][OpenMP] Fix taskloop charbox privatization
Load by-value privatizer arguments from task context storage
when lowering taskloop duplication and firstprivate copy
regions. This keeps pointer-based privatizers unchanged
while allowing descriptor values such as lowered Fortran
boxchar values to be passed by value.
It is not at all ideal that boxchars get special treatment.
MLIR to LLVM-IR should not depend upon any particular
frontend, but this is a long-standing TODO. The purpose
of this patch is to fix the reported bug.
Fixes: #205479
Assisted-by: Codex
---
.../OpenMP/OpenMPToLLVMIRTranslation.cpp | 57 ++++++++++++++-----
.../LLVMIR/openmp-taskloop-charbox.mlir | 54 ++++++++++++++++++
2 files changed, 97 insertions(+), 14 deletions(-)
create mode 100644 mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 0f954e384929a..4de38aa505fcc 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -1733,6 +1733,25 @@ findAssociatedValue(Value privateVar, llvm::IRBuilderBase &builder,
return moduleTranslation.lookupValue(privateVar);
}
+// Privatizer region arguments may be by-value even when the available LLVM
+// value is storage for that value, e.g. lowered Fortran boxchar descriptors in
+// task context structs. Materialize the value expected by the region argument
+// while preserving the existing pointer mapping for pointer arguments.
+static llvm::Value *
+materializeRegionArgValue(llvm::IRBuilderBase &builder,
+ LLVM::ModuleTranslation &moduleTranslation,
+ BlockArgument regionArg, llvm::Value *value) {
+ if (!regionArg)
+ return value;
+
+ llvm::Type *regionArgType =
+ moduleTranslation.convertType(regionArg.getType());
+ if (regionArgType->isPointerTy() || !value->getType()->isPointerTy())
+ return value;
+
+ return builder.CreateLoad(regionArgType, value);
+}
+
/// Initialize a single (first)private variable. You probably want to use
/// allocateAndInitPrivateVars instead of this.
/// This returns the private variable which has been initialized. This
@@ -1924,10 +1943,15 @@ static LogicalResult copyFirstPrivateVars(
// copyRegion implements `lhs = rhs`
Region ©Region = decl.getCopyRegion();
- moduleTranslation.mapValue(decl.getCopyMoldArg(), moldVar);
+ llvm::Value *copyMoldVar = materializeRegionArgValue(
+ builder, moduleTranslation, decl.getCopyMoldArg(), moldVar);
+ llvm::Value *copyPrivateVar = materializeRegionArgValue(
+ builder, moduleTranslation, decl.getCopyPrivateArg(), llvmVar);
+
+ moduleTranslation.mapValue(decl.getCopyMoldArg(), copyMoldVar);
// map copyRegion lhs arg
- moduleTranslation.mapValue(decl.getCopyPrivateArg(), llvmVar);
+ moduleTranslation.mapValue(decl.getCopyPrivateArg(), copyPrivateVar);
// in-place convert copy region
if (failed(inlineConvertOmpRegions(copyRegion, "omp.private.copy", builder,
@@ -3079,14 +3103,15 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
// initialized character box is yielded by value. Here we need to store the
// yielded value into the private allocation, and load the private
// allocation to match the type expected by region block arguments.
+ llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
if ((privateVarOrErr.get() != llvmPrivateVarAlloc) &&
!mlir::isa<LLVM::LLVMPointerType>(blockArg.getType())) {
builder.CreateStore(privateVarOrErr.get(), llvmPrivateVarAlloc);
// Load it so we have the value pointed to by the GEP
- llvmPrivateVarAlloc = builder.CreateLoad(privateVarOrErr.get()->getType(),
- llvmPrivateVarAlloc);
+ llvmPrivateVar = builder.CreateLoad(privateVarOrErr.get()->getType(),
+ llvmPrivateVarAlloc);
}
- assert(llvmPrivateVarAlloc->getType() ==
+ assert(llvmPrivateVar->getType() ==
moduleTranslation.convertType(blockArg.getType()));
// Mapping blockArg -> llvmPrivateVarAlloc is done inside the body callback
@@ -3561,14 +3586,15 @@ convertOmpTaskloopContextOp(omp::TaskloopContextOp contextOp,
llvm::IRBuilderBase::InsertPointGuard guard(builder);
builder.SetInsertPoint(builder.GetInsertBlock()->getTerminator());
+ llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
if ((privateVarOrErr.get() != llvmPrivateVarAlloc) &&
!mlir::isa<LLVM::LLVMPointerType>(blockArg.getType())) {
builder.CreateStore(privateVarOrErr.get(), llvmPrivateVarAlloc);
// Load it so we have the value pointed to by the GEP
- llvmPrivateVarAlloc = builder.CreateLoad(privateVarOrErr.get()->getType(),
- llvmPrivateVarAlloc);
+ llvmPrivateVar = builder.CreateLoad(privateVarOrErr.get()->getType(),
+ llvmPrivateVarAlloc);
}
- assert(llvmPrivateVarAlloc->getType() ==
+ assert(llvmPrivateVar->getType() ==
moduleTranslation.convertType(blockArg.getType()));
}
@@ -3842,9 +3868,11 @@ convertOmpTaskloopContextOp(omp::TaskloopContextOp contextOp,
assert(llvmPrivateVarAlloc &&
"reads from mold so shouldn't have been skipped");
- llvm::Expected<llvm::Value *> privateVarOrErr =
- initPrivateVar(builder, moduleTranslation, privDecl, mold, blockArg,
- llvmPrivateVarAlloc, builder.GetInsertBlock());
+ llvm::Value *moldArg = materializeRegionArgValue(
+ builder, moduleTranslation, privDecl.getInitMoldArg(), mold);
+ llvm::Expected<llvm::Value *> privateVarOrErr = initPrivateVar(
+ builder, moduleTranslation, privDecl, moldArg, blockArg,
+ llvmPrivateVarAlloc, builder.GetInsertBlock());
if (!privateVarOrErr)
return privateVarOrErr.takeError();
@@ -3855,14 +3883,15 @@ convertOmpTaskloopContextOp(omp::TaskloopContextOp contextOp,
// initialized character box is yielded by value. Here we need to store
// the yielded value into the private allocation, and load the private
// allocation to match the type expected by region block arguments.
+ llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
if ((privateVarOrErr.get() != llvmPrivateVarAlloc) &&
!mlir::isa<LLVM::LLVMPointerType>(blockArg.getType())) {
builder.CreateStore(privateVarOrErr.get(), llvmPrivateVarAlloc);
// Load it so we have the value pointed to by the GEP
- llvmPrivateVarAlloc = builder.CreateLoad(
- privateVarOrErr.get()->getType(), llvmPrivateVarAlloc);
+ llvmPrivateVar = builder.CreateLoad(privateVarOrErr.get()->getType(),
+ llvmPrivateVarAlloc);
}
- assert(llvmPrivateVarAlloc->getType() ==
+ assert(llvmPrivateVar->getType() ==
moduleTranslation.convertType(blockArg.getType()));
// Mapping blockArg -> llvmPrivateVarAlloc is done inside the body
diff --git a/mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir b/mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir
new file mode 100644
index 0000000000000..a20cc0149a453
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir
@@ -0,0 +1,54 @@
+// RUN: mlir-translate --mlir-to-llvmir %s | FileCheck %s
+
+module {
+ llvm.func @touch(!llvm.ptr, i64)
+
+ omp.private {type = firstprivate} @box_firstprivate : !llvm.struct<(ptr, i64)> init {
+ ^bb0(%arg0: !llvm.struct<(ptr, i64)>, %arg1: !llvm.struct<(ptr, i64)>):
+ %0 = llvm.extractvalue %arg0[0] : !llvm.struct<(ptr, i64)>
+ %1 = llvm.extractvalue %arg0[1] : !llvm.struct<(ptr, i64)>
+ %2 = llvm.mlir.undef : !llvm.struct<(ptr, i64)>
+ %3 = llvm.insertvalue %0, %2[0] : !llvm.struct<(ptr, i64)>
+ %4 = llvm.insertvalue %1, %3[1] : !llvm.struct<(ptr, i64)>
+ omp.yield(%4 : !llvm.struct<(ptr, i64)>)
+ } copy {
+ ^bb0(%arg0: !llvm.struct<(ptr, i64)>, %arg1: !llvm.struct<(ptr, i64)>):
+ %0 = llvm.extractvalue %arg0[0] : !llvm.struct<(ptr, i64)>
+ %1 = llvm.extractvalue %arg0[1] : !llvm.struct<(ptr, i64)>
+ llvm.call @touch(%0, %1) : (!llvm.ptr, i64) -> ()
+ omp.yield(%arg0 : !llvm.struct<(ptr, i64)>)
+ } dealloc {
+ ^bb0(%arg0: !llvm.struct<(ptr, i64)>):
+ %0 = llvm.extractvalue %arg0[0] : !llvm.struct<(ptr, i64)>
+ %1 = llvm.extractvalue %arg0[1] : !llvm.struct<(ptr, i64)>
+ llvm.call @touch(%0, %1) : (!llvm.ptr, i64) -> ()
+ omp.yield
+ }
+
+ llvm.func @test(%arg0: !llvm.ptr, %arg1: i64) {
+ %0 = llvm.mlir.undef : !llvm.struct<(ptr, i64)>
+ %1 = llvm.insertvalue %arg0, %0[0] : !llvm.struct<(ptr, i64)>
+ %2 = llvm.insertvalue %arg1, %1[1] : !llvm.struct<(ptr, i64)>
+ %c1 = llvm.mlir.constant(1 : i32) : i32
+ %c2 = llvm.mlir.constant(2 : i32) : i32
+ omp.taskloop.context private(@box_firstprivate %2 -> %arg2 : !llvm.struct<(ptr, i64)>) {
+ omp.taskloop.wrapper {
+ omp.loop_nest (%arg3) : i32 = (%c1) to (%c2) inclusive step (%c1) {
+ %3 = llvm.extractvalue %arg2[0] : !llvm.struct<(ptr, i64)>
+ %4 = llvm.extractvalue %arg2[1] : !llvm.struct<(ptr, i64)>
+ llvm.call @touch(%3, %4) : (!llvm.ptr, i64) -> ()
+ omp.yield
+ }
+ }
+ omp.terminator
+ }
+ llvm.return
+ }
+}
+
+// CHECK-LABEL: define void @test(
+// CHECK: call void @__kmpc_taskloop(
+
+// CHECK-LABEL: define internal void @omp_taskloop_dup(
+// CHECK: load { ptr, i64 }, ptr
+// CHECK: call void @touch(
>From 626ccf20e8b287a3c4b2954e32d0e8d4981a5632 Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Thu, 25 Jun 2026 15:12:07 +0100
Subject: [PATCH 2/3] Avoid NDEBUG task privatizer warning
---
.../LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 4de38aa505fcc..054c04eb745d1 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -3103,7 +3103,7 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
// initialized character box is yielded by value. Here we need to store the
// yielded value into the private allocation, and load the private
// allocation to match the type expected by region block arguments.
- llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
+ [[maybe_unused]] llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
if ((privateVarOrErr.get() != llvmPrivateVarAlloc) &&
!mlir::isa<LLVM::LLVMPointerType>(blockArg.getType())) {
builder.CreateStore(privateVarOrErr.get(), llvmPrivateVarAlloc);
@@ -3586,7 +3586,7 @@ convertOmpTaskloopContextOp(omp::TaskloopContextOp contextOp,
llvm::IRBuilderBase::InsertPointGuard guard(builder);
builder.SetInsertPoint(builder.GetInsertBlock()->getTerminator());
- llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
+ [[maybe_unused]] llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
if ((privateVarOrErr.get() != llvmPrivateVarAlloc) &&
!mlir::isa<LLVM::LLVMPointerType>(blockArg.getType())) {
builder.CreateStore(privateVarOrErr.get(), llvmPrivateVarAlloc);
@@ -3883,7 +3883,7 @@ convertOmpTaskloopContextOp(omp::TaskloopContextOp contextOp,
// initialized character box is yielded by value. Here we need to store
// the yielded value into the private allocation, and load the private
// allocation to match the type expected by region block arguments.
- llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
+ [[maybe_unused]] llvm::Value *llvmPrivateVar = llvmPrivateVarAlloc;
if ((privateVarOrErr.get() != llvmPrivateVarAlloc) &&
!mlir::isa<LLVM::LLVMPointerType>(blockArg.getType())) {
builder.CreateStore(privateVarOrErr.get(), llvmPrivateVarAlloc);
>From 85aee0132adb2f758a2998421714fe8d1c9d9cec Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Thu, 25 Jun 2026 15:22:02 +0100
Subject: [PATCH 3/3] Tighten taskloop charbox test
---
mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir b/mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir
index a20cc0149a453..dff7cee1811fd 100644
--- a/mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir
@@ -50,5 +50,6 @@ module {
// CHECK: call void @__kmpc_taskloop(
// CHECK-LABEL: define internal void @omp_taskloop_dup(
-// CHECK: load { ptr, i64 }, ptr
+// CHECK: %[[MOLD:.*]] = load { ptr, i64 }, ptr
+// CHECK: extractvalue { ptr, i64 } %[[MOLD]], 0
// CHECK: call void @touch(
More information about the Mlir-commits
mailing list