[Mlir-commits] [mlir] [MLIR][OpenMP] Fix taskloop charbox privatization (PR #205568)
Tom Eccles
llvmlistbot at llvm.org
Wed Jun 24 07:07:59 PDT 2026
https://github.com/tblah created https://github.com/llvm/llvm-project/pull/205568
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
>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] [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(
More information about the Mlir-commits
mailing list