[flang][OpenMP] Lower target in_reduction#199967
Conversation
0b87745 to
d7684c1
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds partial lowering and LLVM IR translation support for OpenMP target in_reduction, wiring target-region uses to task reduction-private storage for supported host-fallback cases.
Changes:
- Preserves
in_reductionoperands inomp::TargetOp::build(TargetOperands). - Adds MLIR OpenMP translation handling and diagnostics for supported/unsupported
omp.target in_reductionforms. - Updates Flang lowering/tests to emit implicit target map entries for
in_reductionlist items.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp |
Preserves target in_reduction clause operands in the builder path. |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp |
Adds target in_reduction validation and runtime private-storage lookup during translation. |
mlir/test/Target/LLVMIR/openmp-todo.mlir |
Updates TODO coverage for unsupported target in_reduction cases. |
mlir/test/Target/LLVMIR/openmp-target-in-reduction.mlir |
Adds LLVM IR translation coverage for supported host-fallback target in_reduction. |
flang/lib/Lower/OpenMP/OpenMP.cpp |
Lowers target in_reduction clauses and force-adds implicit map entries. |
flang/test/Lower/OpenMP/Todo/target-inreduction.f90 |
Removes obsolete TODO test. |
flang/test/Lower/OpenMP/target-inreduction.f90 |
Adds lowering coverage for used target in_reduction variables. |
flang/test/Lower/OpenMP/target-inreduction-unused.f90 |
Adds lowering coverage for unused target in_reduction variables. |
llvmorg-github-actions
Bot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-mlir-llvm Author: Sairudra More (Saieiei) ChangesThis is stacked on the existing taskgroup/taskloop reduction work. This patch teaches Flang lowering and MLIR OpenMP translation to carry The translation looks up the task reduction-private storage with: and binds the target region argument to that private pointer, so uses inside the region do not continue referring to the original variable. The patch also fixes the For Flang lowering, Fixes #199904 Patch is 21.39 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/199967.diff 8 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 7cb7e379eb503..099220acf8102 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1873,6 +1873,7 @@ genTargetClauses(lower::AbstractConverter &converter,
mlir::omp::TargetOperands &clauseOps,
DefaultMapsTy &defaultMaps,
llvm::SmallVectorImpl<Object> &hasDeviceAddrObjects,
+ llvm::SmallVectorImpl<Object> &inReductionObjects,
llvm::SmallVectorImpl<Object> &isDevicePtrObjects,
llvm::SmallVectorImpl<Object> &mapObjects) {
ClauseProcessor cp(converter, semaCtx, clauses);
@@ -1887,13 +1888,14 @@ genTargetClauses(lower::AbstractConverter &converter,
hostEvalInfo->collectValues(clauseOps.hostEvalVars);
}
cp.processIf(llvm::omp::Directive::OMPD_target, clauseOps);
+ cp.processInReduction(loc, clauseOps, inReductionObjects);
cp.processIsDevicePtr(stmtCtx, clauseOps, isDevicePtrObjects);
cp.processMap(loc, stmtCtx, clauseOps, llvm::omp::Directive::OMPD_unknown,
&mapObjects);
cp.processNowait(clauseOps);
cp.processThreadLimit(stmtCtx, clauseOps);
- cp.processTODO<clause::Allocate, clause::InReduction, clause::UsesAllocators>(
+ cp.processTODO<clause::Allocate, clause::UsesAllocators>(
loc, llvm::omp::Directive::OMPD_target);
// `target private(..)` is only supported in delayed privatization mode.
@@ -2932,10 +2934,10 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
mlir::omp::TargetOperands clauseOps;
DefaultMapsTy defaultMaps;
llvm::SmallVector<Object> mapObjects, hasDeviceAddrObjects,
- isDevicePtrObjects;
+ inReductionObjects, isDevicePtrObjects;
genTargetClauses(converter, semaCtx, symTable, stmtCtx, eval, item->clauses,
loc, clauseOps, defaultMaps, hasDeviceAddrObjects,
- isDevicePtrObjects, mapObjects);
+ inReductionObjects, isDevicePtrObjects, mapObjects);
if (!isDevicePtrObjects.empty()) {
// is_device_ptr maps get duplicated so the clause and synthesized
@@ -3110,6 +3112,16 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
};
lower::pft::visitAllSymbols(eval, captureImplicitMap);
+ // OpenMP requires `in_reduction` list items on `target` to be implicitly
+ // data-mapped. The body-symbol walk above only catches list items that are
+ // referenced inside the target region; force-capture the rest so the MLIR
+ // -> LLVM IR translation can rely on every in_reduction operand being
+ // present in `map_entries`. `captureImplicitMap` is a no-op for symbols
+ // already covered by an explicit map/has_device_addr/is_device_ptr.
+ for (const Object &object : inReductionObjects)
+ if (const semantics::Symbol *sym = object.sym())
+ captureImplicitMap(*sym);
+
auto targetOp = mlir::omp::TargetOp::create(firOpBuilder, loc, clauseOps);
llvm::SmallVector<mlir::Value> hasDeviceAddrBaseValues, mapBaseValues;
@@ -3120,7 +3132,8 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
args.hasDeviceAddr.objects = hasDeviceAddrObjects;
args.hasDeviceAddr.vars = hasDeviceAddrBaseValues;
args.hostEvalVars = clauseOps.hostEvalVars;
- // TODO: Add in_reduction syms and vars.
+ args.inReduction.objects = inReductionObjects;
+ args.inReduction.vars = clauseOps.inReductionVars;
args.map.objects = mapObjects;
args.map.vars = mapBaseValues;
args.priv.objects = makeObjects(dsp.getDelayedPrivSymbols());
diff --git a/flang/test/Lower/OpenMP/Todo/target-inreduction.f90 b/flang/test/Lower/OpenMP/Todo/target-inreduction.f90
deleted file mode 100644
index e5a9cffac5a11..0000000000000
--- a/flang/test/Lower/OpenMP/Todo/target-inreduction.f90
+++ /dev/null
@@ -1,15 +0,0 @@
-! RUN: %not_todo_cmd bbc -emit-fir -fopenmp -fopenmp-version=50 -o - %s 2>&1 | FileCheck %s
-! RUN: %not_todo_cmd %flang_fc1 -emit-fir -fopenmp -fopenmp-version=50 -o - %s 2>&1 | FileCheck %s
-
-!===============================================================================
-! `mergeable` clause
-!===============================================================================
-
-! CHECK: not yet implemented: Unhandled clause IN_REDUCTION in TARGET construct
-subroutine omp_target_inreduction()
- integer i
- i = 0
- !$omp target in_reduction(+:i)
- i = i + 1
- !$omp end target
-end subroutine omp_target_inreduction
diff --git a/flang/test/Lower/OpenMP/target-inreduction-unused.f90 b/flang/test/Lower/OpenMP/target-inreduction-unused.f90
new file mode 100644
index 0000000000000..6831136307a59
--- /dev/null
+++ b/flang/test/Lower/OpenMP/target-inreduction-unused.f90
@@ -0,0 +1,27 @@
+! RUN: bbc -emit-hlfir -fopenmp -fopenmp-version=50 -o - %s 2>&1 | FileCheck %s
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 -o - %s 2>&1 | FileCheck %s
+
+! Per the OpenMP spec, an in_reduction list item on a target construct is
+! implicitly data-mapped. The lowering must not rely on the variable being
+! referenced inside the target body to discover that map: here `i` only
+! appears in the in_reduction clause and is never read or written inside
+! the region. Verify that an omp.map.info for `i` is still emitted and
+! flows into the omp.target's map_entries.
+
+!CHECK-LABEL: func.func @_QPomp_target_in_reduction_unused()
+!CHECK: %[[IDECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFomp_target_in_reduction_unusedEi"}
+!CHECK: %[[IMAP:.*]] = omp.map.info var_ptr(%[[IDECL]]#1 : !fir.ref<i32>, i32) {{.*}} {name = "i"}
+!CHECK: omp.target in_reduction(@{{[^ ]+}} %[[IDECL]]#0 -> %{{[^ ]+}} : !fir.ref<i32>)
+!CHECK-SAME: map_entries(%[[IMAP]] -> %{{[^ ]+}} : !fir.ref<i32>)
+
+subroutine omp_target_in_reduction_unused()
+ interface
+ subroutine sub()
+ end subroutine
+ end interface
+ integer i
+ i = 0
+ !$omp target in_reduction(+:i)
+ call sub()
+ !$omp end target
+end subroutine omp_target_in_reduction_unused
diff --git a/flang/test/Lower/OpenMP/target-inreduction.f90 b/flang/test/Lower/OpenMP/target-inreduction.f90
new file mode 100644
index 0000000000000..0576e9099e19e
--- /dev/null
+++ b/flang/test/Lower/OpenMP/target-inreduction.f90
@@ -0,0 +1,28 @@
+! RUN: bbc -emit-hlfir -fopenmp -fopenmp-version=50 -o - %s 2>&1 | FileCheck %s
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 -o - %s 2>&1 | FileCheck %s
+
+! Verify that in_reduction on a target construct is lowered to an
+! omp.target with both an in_reduction clause and an implicit map_entries
+! entry for the same variable. The implicit map captures the original
+! pointer into the target region so the MLIR -> LLVM IR translation can
+! pass it to __kmpc_task_reduction_get_th_data.
+
+!CHECK-LABEL: omp.declare_reduction
+!CHECK-SAME: @[[RED_I32_NAME:.*]] : i32 init {
+
+!CHECK-LABEL: func.func @_QPomp_target_in_reduction()
+!CHECK: %[[IDECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFomp_target_in_reductionEi"}
+!CHECK: %[[IMAP:.*]] = omp.map.info var_ptr(%[[IDECL]]#1 : !fir.ref<i32>, i32) {{.*}} {name = "i"}
+!CHECK: omp.target in_reduction(@[[RED_I32_NAME]] %[[IDECL]]#0 -> %[[INARG:[^ ]+]] : !fir.ref<i32>)
+!CHECK-SAME: map_entries(%[[IMAP]] -> %{{[^ ]+}} : !fir.ref<i32>)
+!CHECK: hlfir.declare %[[INARG]]
+!CHECK: omp.terminator
+!CHECK: }
+
+subroutine omp_target_in_reduction()
+ integer i
+ i = 0
+ !$omp target in_reduction(+:i)
+ i = i + 1
+ !$omp end target
+end subroutine omp_target_in_reduction
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index 7cef23bdfef18..8836ebce03349 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2545,8 +2545,7 @@ LogicalResult TargetUpdateOp::verify() {
void TargetOp::build(OpBuilder &builder, OperationState &state,
const TargetOperands &clauses) {
MLIRContext *ctx = builder.getContext();
- // TODO Store clauses in op: allocateVars, allocatorVars, inReductionVars,
- // inReductionByref, inReductionSyms.
+ // TODO Store clauses in op: allocateVars, allocatorVars.
TargetOp::build(
builder, state, /*allocate_vars=*/{}, /*allocator_vars=*/{}, clauses.bare,
makeArrayAttr(ctx, clauses.dependKinds), clauses.dependVars,
@@ -2554,9 +2553,10 @@ void TargetOp::build(OpBuilder &builder, OperationState &state,
clauses.device, clauses.dynGroupprivateAccessGroup,
clauses.dynGroupprivateFallback, clauses.dynGroupprivateSize,
clauses.hasDeviceAddrVars, clauses.hostEvalVars, clauses.ifExpr,
- /*in_reduction_vars=*/{}, /*in_reduction_byref=*/nullptr,
- /*in_reduction_syms=*/nullptr, clauses.isDevicePtrVars, clauses.mapVars,
- clauses.nowait, clauses.privateVars,
+ clauses.inReductionVars,
+ makeDenseBoolArrayAttr(ctx, clauses.inReductionByref),
+ makeArrayAttr(ctx, clauses.inReductionSyms), clauses.isDevicePtrVars,
+ clauses.mapVars, clauses.nowait, clauses.privateVars,
makeArrayAttr(ctx, clauses.privateSyms), clauses.privateNeedsBarrier,
clauses.threadLimitVars,
/*private_maps=*/nullptr);
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 1120d9fc38d0a..2ef23a80577d8 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -490,7 +490,11 @@ static LogicalResult checkImplementationStatus(Operation &op) {
.Case([&](omp::TargetOp op) {
checkAllocate(op, result);
checkBare(op, result);
- checkInReduction(op, result);
+ // in_reduction(byref(...)) on target is not implemented yet. Other
+ // unsupported in_reduction shapes (cleanup region, two-argument
+ // initializer, missing combiner) and the device-side / offload-entry
+ // cases are diagnosed inline in convertOmpTarget.
+ checkInReductionByref(op, result);
checkThreadLimit(op, result);
})
.Default([](Operation &) {
@@ -8208,6 +8212,61 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
bool isOffloadEntry =
isTargetDevice || !ompBuilder->Config.TargetTriples.empty();
+ // Validate and resolve in_reduction clauses on omp.target. We currently
+ // only support the non-offload host-fallback path: the per-task private
+ // pointer is obtained by calling __kmpc_task_reduction_get_th_data inside
+ // the to-be-outlined target task body. Threading that pointer through the
+ // device kernel argument list is left as follow-up work.
+ SmallVector<llvm::Value *> inRedOrigPtrs;
+ if (!targetOp.getInReductionVars().empty()) {
+ if (isTargetDevice || isOffloadEntry)
+ return opInst.emitError(
+ "not yet implemented: in_reduction clause on omp.target with "
+ "offload / target-device compilation");
+ if (auto inRedSyms = targetOp.getInReductionSyms()) {
+ for (auto sym : inRedSyms->getAsRange<SymbolRefAttr>()) {
+ auto decl =
+ SymbolTable::lookupNearestSymbolFrom<omp::DeclareReductionOp>(
+ targetOp, sym);
+ if (!decl)
+ return targetOp.emitError()
+ << "failed to resolve in_reduction declare_reduction symbol "
+ << sym.getRootReference() << " on omp.target";
+ if (decl.getInitializerRegion().front().getNumArguments() != 1)
+ return targetOp.emitError()
+ << "not yet implemented: in_reduction with two-argument "
+ "initializer on omp.target";
+ if (!decl.getCleanupRegion().empty())
+ return targetOp.emitError()
+ << "not yet implemented: in_reduction with cleanup region "
+ "on omp.target";
+ // The reduction combiner region is intentionally not required here:
+ // the in_reduction lowering on omp.target only locates the per-task
+ // private storage via __kmpc_task_reduction_get_th_data. The combiner
+ // is owned by the enclosing taskgroup's task_reduction registration.
+ }
+ }
+ // Each in_reduction variable must also be captured by the target via a
+ // map_entries entry referring to the same outer SSA value. OMPIRBuilder
+ // outlines the target body and only rewires uses of values that enter
+ // the kernel through the map-derived input set. The runtime call below
+ // uses that same outer SSA value as its `orig` argument, so without a
+ // matching map entry the outlined kernel would reference a value defined
+ // in the host function and fail IR verification.
+ llvm::SmallPtrSet<Value, 4> mappedVarPtrs;
+ for (Value mapV : targetOp.getMapVars())
+ if (auto mapInfo = mapV.getDefiningOp<omp::MapInfoOp>())
+ mappedVarPtrs.insert(mapInfo.getVarPtr());
+ inRedOrigPtrs.reserve(targetOp.getInReductionVars().size());
+ for (Value v : targetOp.getInReductionVars()) {
+ if (!mappedVarPtrs.contains(v))
+ return targetOp.emitError()
+ << "not yet implemented: in_reduction variable on omp.target "
+ "must also be captured by a matching map_entries entry";
+ inRedOrigPtrs.push_back(moduleTranslation.lookupValue(v));
+ }
+ }
+
// For some private variables, the MapsForPrivatizedVariablesPass
// creates MapInfoOp instances. Go through the private variables and
// the mapped variables so that during codegeneration we are able
@@ -8320,6 +8379,36 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
targetOp.getPrivateNeedsBarrier(), &mappedPrivateVars)))
return llvm::make_error<PreviouslyReportedError>();
+ // Map in_reduction block arguments to the per-task private storage
+ // returned by __kmpc_task_reduction_get_th_data. The lookup must run
+ // inside the target task body so the gtid corresponds to the executing
+ // thread. The descriptor argument is NULL: the runtime walks enclosing
+ // taskgroups to locate the matching task_reduction registration for
+ // `origPtr`. Mirrors the in_reduction handling on omp.taskloop.context.
+ ArrayRef<BlockArgument> inRedBlockArgs = argIface.getInReductionBlockArgs();
+ if (!inRedBlockArgs.empty()) {
+ llvm::OpenMPIRBuilder &ompB = *moduleTranslation.getOpenMPBuilder();
+ llvm::Module *m = moduleTranslation.getLLVMModule();
+ llvm::LLVMContext &llvmCtx = m->getContext();
+ uint32_t srcLocSize;
+ llvm::Constant *srcLocStr = ompB.getOrCreateDefaultSrcLocStr(srcLocSize);
+ llvm::Value *bodyIdent = ompB.getOrCreateIdent(srcLocStr, srcLocSize);
+ llvm::Function *gtidFn = ompB.getOrCreateRuntimeFunctionPtr(
+ llvm::omp::OMPRTL___kmpc_global_thread_num);
+ llvm::Value *bodyGtid =
+ builder.CreateCall(gtidFn, {bodyIdent}, "omp_global_thread_num");
+ llvm::FunctionCallee getThData = ompB.getOrCreateRuntimeFunction(
+ *m, llvm::omp::OMPRTL___kmpc_task_reduction_get_th_data);
+ llvm::Type *ptrTy = llvm::PointerType::getUnqual(llvmCtx);
+ llvm::Value *nullDesc = llvm::ConstantPointerNull::get(ptrTy);
+ for (auto [blockArg, origPtr] :
+ llvm::zip_equal(inRedBlockArgs, inRedOrigPtrs)) {
+ llvm::Value *priv = builder.CreateCall(
+ getThData, {bodyGtid, nullDesc, origPtr}, "omp.inred.priv");
+ moduleTranslation.mapValue(blockArg, priv);
+ }
+ }
+
LLVM::ModuleTranslation::SaveStack<OpenMPAllocStackFrame> frame(
moduleTranslation, allocaIP, deallocBlocks);
llvm::Expected<llvm::BasicBlock *> exitBlock = convertOmpOpRegions(
diff --git a/mlir/test/Target/LLVMIR/openmp-target-in-reduction.mlir b/mlir/test/Target/LLVMIR/openmp-target-in-reduction.mlir
new file mode 100644
index 0000000000000..2b3cfd514d82e
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/openmp-target-in-reduction.mlir
@@ -0,0 +1,50 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+// in_reduction on omp.target: the in_reduction variable is also captured
+// into the target region as a map entry (the Flang front-end emits this
+// implicit map). Inside the outlined target body the captured pointer is
+// passed to __kmpc_task_reduction_get_th_data with a NULL descriptor;
+// the runtime walks enclosing taskgroups to locate the matching
+// task_reduction registration. The returned pointer is bound to the
+// in_reduction region block argument so subsequent loads/stores inside
+// the region use the private copy.
+
+omp.declare_reduction @add_i32 : i32
+init {
+^bb0(%arg0: i32):
+ %c0 = llvm.mlir.constant(0 : i32) : i32
+ omp.yield(%c0 : i32)
+}
+combiner {
+^bb0(%arg0: i32, %arg1: i32):
+ %s = llvm.add %arg0, %arg1 : i32
+ omp.yield(%s : i32)
+}
+
+llvm.func @target_inreduction(%x : !llvm.ptr) {
+ %m = omp.map.info var_ptr(%x : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr
+ omp.target in_reduction(@add_i32 %x -> %prv : !llvm.ptr) map_entries(%m -> %marg : !llvm.ptr) {
+ %v = llvm.load %prv : !llvm.ptr -> i32
+ %c1 = llvm.mlir.constant(1 : i32) : i32
+ %s = llvm.add %v, %c1 : i32
+ llvm.store %s, %prv : i32, !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+}
+
+// The host stub forwards the captured pointer into the outlined target
+// kernel.
+// CHECK-LABEL: define void @target_inreduction(
+// CHECK: call void @__omp_offloading_{{.*}}_target_inreduction_{{.*}}(ptr %{{.+}}, ptr null)
+
+// In the outlined target body the in_reduction private pointer is
+// obtained from the runtime using the captured original pointer; that
+// pointer is then the base of the load and store inside the region.
+// CHECK-LABEL: define internal void @__omp_offloading_{{.*}}_target_inreduction_
+// CHECK-SAME: (ptr %[[CAPT:.+]], ptr %{{.+}})
+// CHECK: %[[GTID:.+]] = call i32 @__kmpc_global_thread_num(
+// CHECK: %[[PRIV:.+]] = call ptr @__kmpc_task_reduction_get_th_data(i32 %[[GTID]], ptr null, ptr %[[CAPT]])
+// CHECK: %[[LOADED:.+]] = load i32, ptr %[[PRIV]]
+// CHECK: %[[SUM:.+]] = add i32 %[[LOADED]], 1
+// CHECK: store i32 %[[SUM]], ptr %[[PRIV]]
diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir
index a84da99458fd1..926ab48503acc 100644
--- a/mlir/test/Target/LLVMIR/openmp-todo.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir
@@ -190,10 +190,90 @@ atomic {
llvm.atomicrmw fadd %arg2, %2 monotonic : !llvm.ptr, f32
omp.yield
}
-llvm.func @target_in_reduction(%x : !llvm.ptr) {
- // expected-error@below {{not yet implemented: Unhandled clause in_reduction in omp.target operation}}
+llvm.func @target_in_reduction_byref(%x : !llvm.ptr) {
+ // expected-error@below {{not yet implemented: Unhandled clause in_reduction with byref modifier in omp.target operation}}
// expected-error@below {{LLVM Translation failed for operation: omp.target}}
- omp.target in_reduction(@add_f32 %x -> %prv : !llvm.ptr) {
+ omp.target in_reduction(byref @add_f32 %x -> %prv : !llvm.ptr) {
+ omp.terminator
+ }
+ llvm.return
+}
+
+// -----
+
+omp.declare_reduction @add_cleanup_f32 : f32
+init {
+^bb0(%arg: f32):
+ %0 = llvm.mlir.constant(0.0 : f32) : f32
+ omp.yield (%0 : f32)
+}
+combiner {
+^bb1(%arg0: f32, %arg1: f32):
+ %1 = llvm.fadd %arg0, %arg1 : f32
+ omp.yield (%1 : f32)
+}
+cleanup {
+^bb2(%arg2: f32):
+ omp.yield
+}
+llvm.func @target_in_reduction_cleanup(%x : !llvm.ptr) {
+ // expected-error@below {{not yet implemented: in_reduction with cleanup region on omp.target}}
+ // expected-error@below {{LLVM Translation failed for operation: omp.target}}
+ omp.target in_reduction(@add_cleanup_f32 %x -> %prv : !llvm.ptr) {
+ omp.terminator
+ }
+ llvm.return
+}
+
+// -----
+
+omp.declare_reduction @add_two_arg_init_i32 : !llvm.ptr alloc {
+^bb0(%arg: !llvm.ptr):
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr
+ omp.yield(%1 : !llvm.ptr)
+} init {
+^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr):
+ %0 = llvm.mlir.constant(0 : i32) : i32
+ llvm.store %0, %arg1 : i32, !llvm.ptr
+ omp.yield(%arg1 : !llvm.ptr)
+} combiner {
+^bb1(%arg0: !llvm.ptr, %arg1: !llvm.pt...
[truncated]
|
d7684c1 to
976469c
Compare
Saieiei
changed the title
976469c to
22ced91
Compare
22ced91 to
753008d
Compare
753008d to
960e149
Compare
d8a77fa to
1ca9edc
Compare
|
Sorry for the noisy rebase/retarget churn here. After rebasing this PR onto The PR is now clean again: base is |
| return targetOp.emitError() | ||
| << "not yet implemented: in_reduction variable on omp.target " | ||
| "must also be captured by a matching map_entries entry"; |
There was a problem hiding this comment.
I think the fact that an in_reduction operand must also be a map argument is something that ought to be verified regardless of which one of the two representation alternatives we choose. Either way, I'm happier with the block argument-less version that's currently implemented in this patch.
|
There are exceptions but Jean and Slava don't usually review OpenMP changes. Kiran and I don't usually review target offload changes. |
Right, sorry for not removing myself here, I am happy to review whenever an OpenMP patch is touching common lowering utilities, otherwise I am not sharp enough with regards to OpenMP to be of much use in the reviews (sometimes when randomly reading an OpenMP patch and seeing something odd, I will still ask about it, but I am not the best person to approve). |
I tried moving this into the For Flang-generated HLFIR, the A verifier check based on direct SSA identity therefore rejects valid Flang-generated cases, including the basic target Open to alternatives if there is a dialect-level way to express this relationship without depending on HLFIR/FIR details. |
skatrak
left a comment
skatrak
left a comment
There was a problem hiding this comment.
Thank you for the quick update, there are some smaller details left. Otherwise, I think this is almost ready.
For Flang-generated HLFIR, the in_reduction operand and the matching map.info var_ptr can come from different results of the same hlfir.declare: the in_reduction operand uses result 0, while the map.info var_ptr uses result 1. They are distinct SSA values at the OpenMP verifier point and only become the same underlying address after FIR/LLVM lowering.
I see... That's not great, because not being able to make a direct connection between an in_reduction and map value means we can produce broken MLIR that cannot be translated to LLVM IR. The best we can do at the moment is to check in the verifier that all in_reduction operands are produced by the same operation as the var_ptr of at least one of the map_entries of the same operation. And then, where you're currently making the "not yet implemented: in_reduction variable on omp.target must also be captured by a matching map_entries entry" check put an assert instead. That'd be my suggestion in this case.
|
Hi @skatrak, I rebased the PR onto current For the latest |
MattPD
left a comment
MattPD
left a comment
There was a problem hiding this comment.
Re-reviewed after the round-3 changes. The OMPIRBuilder move, the device path, and the test additions look good, and my earlier two points are addressed. Two observations on the new COMMON-block guard, both about sibling cases of the same hazard.
|
ping @skatrak |
skatrak
left a comment
skatrak
left a comment
There was a problem hiding this comment.
I still have a couple of comments on implementation details.
| return targetOp.emitError() | ||
| << "not yet implemented: in_reduction variable on omp.target " | ||
| "must also be captured by a matching map_entries entry"; |
There was a problem hiding this comment.
This is still not addressed, I'll copy again my previous suggestion:
The best we can do at the moment is to check in the verifier that all in_reduction operands are produced by the same operation as the var_ptr of at least one of the map_entries of the same operation. And then, where you're currently making the "not yet implemented: in_reduction variable on omp.target must also be captured by a matching map_entries entry" check put an assert instead. That'd be my suggestion in this case.
|
Done. I moved the target I also moved the missing matching |
|
Just a tiny nit: the commit message says "Unsupported device/offload-entry ... remain diagnosed", but this revision removes the offload-entry rejection (on the target device an in_reduction item is now handled as a plain map(tofrom)). Might be worth updating that line to name what's still diagnosed: the byref modifier, a two-argument initializer, a cleanup region, and the Flang COMMON/EQUIVALENCE/privatized cases. |
|
Done! |
|
Ping @skatrak |
skatrak
left a comment
skatrak
left a comment
There was a problem hiding this comment.
LGTM, no need for another review by me. However, please do address the last nits before merging. Thank you!
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
|
CI failures are unrelated to this PR: both Linux and AArch64 fail only in lldb-api.lang/cpp/std-function-step-into-callable. |
Enable host lowering for target in_reduction in Flang and MLIR OpenMP translation. Model target in_reduction through the matching map entry, force address-preserving implicit mapping for Flang in_reduction list items, and emit the host-side task-reduction lookup with __kmpc_task_reduction_get_th_data. The runtime entry point takes and returns a generic, default-address-space pointer, so normalize a non-default-address-space captured pointer to the generic address space before the call and cast the returned private pointer back to the map block argument's address space, mirroring the in_reduction handling on omp.taskloop. On the target device, in_reduction is handled as a regular map(tofrom) variable. The byref modifier, two-argument initializers, cleanup regions, and the remaining Flang COMMON/EQUIVALENCE/privatized-variable cases continue to be diagnosed. Add Flang lowering, MLIR verifier/translation, and LLVM IR tests for the supported host path, including a non-default-address-space case, and the remaining unsupported cases.
This patch carries
in_reductionthroughomp.targetfor host execution.The lowering looks up the task-reduction private storage with
__kmpc_task_reduction_get_th_dataand binds the target region argument to that private pointer. This makes uses inside the target region refer to the task-private reduction storage instead of continuing to use the original variable.This also fixes the
omp::TargetOp::build(TargetOperands)path soin_reductionoperands are preserved instead of being dropped.On the Flang side,
target in_reductionlist items are added to the target map entries when needed, giving the translation a matching mapped value to rewrite.Stack / review order
This PR is related to the OpenMP task-reduction translation stack:
omp.taskgroup task_reductiontranslationomp.taskloopreduction/in_reductiontranslationomp.task in_reductiontranslationThis PR is a target-specific sibling follow-up:
omp.target in_reductionhost loweringThe intended main review order for the task/taskloop stack is:
#199565 → #199670 → #202611
This PR should be reviewed as the
omp.target in_reductionhost-side lowering work built on the same task-reduction runtime model, not as a dependency of #202611.Addresses #199904.
Assisted-by: Claude Opus 4.8 and ChatGPT 5.5