[Mlir-commits] [mlir] [MLIR][OpenMP] Fix taskloop charbox privatization (PR #205568)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Wed Jun 24 08:30:47 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-flang-openmp
Author: Tom Eccles (tblah)
<details>
<summary>Changes</summary>
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
---
Full diff: https://github.com/llvm/llvm-project/pull/205568.diff
2 Files Affected:
- (modified) mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp (+43-14)
- (added) mlir/test/Target/LLVMIR/openmp-taskloop-charbox.mlir (+54)
``````````diff
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(
``````````
</details>
https://github.com/llvm/llvm-project/pull/205568
More information about the Mlir-commits
mailing list