From 57e389c2a0cac02f29f606f3ed9761858be4d9ff Mon Sep 17 00:00:00 2001 From: Tom Eccles Date: Wed, 2 Jul 2025 15:48:50 +0000 Subject: [PATCH 1/5] [mlir][OpenMP] Don't allow firstprivate for simd This is not allowed by the openmp standard. --- .../OpenMP/OpenMPToLLVMIRTranslation.cpp | 23 ++++++++++++++++++- mlir/test/Target/LLVMIR/openmp-todo.mlir | 23 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index 7b07243c5f843..b307de09fbcc4 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -326,6 +326,25 @@ static LogicalResult checkImplementationStatus(Operation &op) { if (op.getDistScheduleChunkSize()) result = todo("dist_schedule with chunk_size"); }; + auto checkFirstprivate = [&todo](auto op, LogicalResult &result) { + // Firstprivate is not listed as supported by the simd operation in + // OpenMP 6.0. This is here to catch it because it is allowed at the dialect + // level. + if constexpr (std::is_same_v, omp::SimdOp>) { + std::optional privateSyms = op.getPrivateSyms(); + if (!privateSyms) + return; + for (const Attribute &sym : *privateSyms) { + omp::PrivateClauseOp privatizer = + findPrivatizer(op, cast(sym)); + if (privatizer.getDataSharingType() == + omp::DataSharingClauseType::FirstPrivate) { + result = todo("firstprivate"); + break; + } + } + } + }; auto checkHint = [](auto op, LogicalResult &) { if (op.getHint()) op.emitWarning("hint clause discarded"); @@ -441,6 +460,7 @@ static LogicalResult checkImplementationStatus(Operation &op) { checkReduction(op, result); }) .Case([&](omp::SimdOp op) { + checkFirstprivate(op, result); checkLinear(op, result); checkReduction(op, result); }) @@ -2894,7 +2914,8 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder, .failed()) return failure(); - // TODO: no call to copyFirstPrivateVars? + // No call to copyFirstPrivateVars because firstprivate is not allowed on + // SIMD. assert(afterAllocas.get()->getSinglePredecessor()); if (failed(initReductionVars(simdOp, reductionArgs, builder, diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir index 29725a02c075a..e86a91dab873d 100644 --- a/mlir/test/Target/LLVMIR/openmp-todo.mlir +++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir @@ -503,3 +503,26 @@ llvm.func @wsloop_order(%lb : i32, %ub : i32, %step : i32) { } llvm.return } + +// ----- + +omp.private {type = firstprivate} @_QFEp_firstprivate_i32 : i32 copy { +^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr): + %0 = llvm.load %arg0 : !llvm.ptr -> i32 + llvm.store %0, %arg1 : i32, !llvm.ptr + omp.yield(%arg1 : !llvm.ptr) +} +llvm.func @_QQmain() { + %0 = llvm.mlir.constant(1 : i64) : i64 + %1 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr + %3 = llvm.mlir.constant(10 : i32) : i32 + %4 = llvm.mlir.constant(1 : i32) : i32 + // expected-error@below {{not yet implemented: Unhandled clause firstprivate in omp.simd operation}} + // expected-error@below {{LLVM Translation failed for operation: omp.simd}} + omp.simd private(@_QFEp_firstprivate_i32 %1 -> %arg0 : !llvm.ptr) { + omp.loop_nest (%arg2) : i32 = (%4) to (%3) inclusive step (%4) { + omp.yield + } + } + llvm.return +} From 683dc84ff16d4b8eb09181f20822d4bb62ca64e0 Mon Sep 17 00:00:00 2001 From: Tom Eccles Date: Thu, 3 Jul 2025 10:46:49 +0000 Subject: [PATCH 2/5] Revert "[mlir][OpenMP] Don't allow firstprivate for simd" This reverts commit 57e389c2a0cac02f29f606f3ed9761858be4d9ff. --- .../OpenMP/OpenMPToLLVMIRTranslation.cpp | 23 +------------------ mlir/test/Target/LLVMIR/openmp-todo.mlir | 23 ------------------- 2 files changed, 1 insertion(+), 45 deletions(-) diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index b307de09fbcc4..7b07243c5f843 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -326,25 +326,6 @@ static LogicalResult checkImplementationStatus(Operation &op) { if (op.getDistScheduleChunkSize()) result = todo("dist_schedule with chunk_size"); }; - auto checkFirstprivate = [&todo](auto op, LogicalResult &result) { - // Firstprivate is not listed as supported by the simd operation in - // OpenMP 6.0. This is here to catch it because it is allowed at the dialect - // level. - if constexpr (std::is_same_v, omp::SimdOp>) { - std::optional privateSyms = op.getPrivateSyms(); - if (!privateSyms) - return; - for (const Attribute &sym : *privateSyms) { - omp::PrivateClauseOp privatizer = - findPrivatizer(op, cast(sym)); - if (privatizer.getDataSharingType() == - omp::DataSharingClauseType::FirstPrivate) { - result = todo("firstprivate"); - break; - } - } - } - }; auto checkHint = [](auto op, LogicalResult &) { if (op.getHint()) op.emitWarning("hint clause discarded"); @@ -460,7 +441,6 @@ static LogicalResult checkImplementationStatus(Operation &op) { checkReduction(op, result); }) .Case([&](omp::SimdOp op) { - checkFirstprivate(op, result); checkLinear(op, result); checkReduction(op, result); }) @@ -2914,8 +2894,7 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder, .failed()) return failure(); - // No call to copyFirstPrivateVars because firstprivate is not allowed on - // SIMD. + // TODO: no call to copyFirstPrivateVars? assert(afterAllocas.get()->getSinglePredecessor()); if (failed(initReductionVars(simdOp, reductionArgs, builder, diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir index e86a91dab873d..29725a02c075a 100644 --- a/mlir/test/Target/LLVMIR/openmp-todo.mlir +++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir @@ -503,26 +503,3 @@ llvm.func @wsloop_order(%lb : i32, %ub : i32, %step : i32) { } llvm.return } - -// ----- - -omp.private {type = firstprivate} @_QFEp_firstprivate_i32 : i32 copy { -^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr): - %0 = llvm.load %arg0 : !llvm.ptr -> i32 - llvm.store %0, %arg1 : i32, !llvm.ptr - omp.yield(%arg1 : !llvm.ptr) -} -llvm.func @_QQmain() { - %0 = llvm.mlir.constant(1 : i64) : i64 - %1 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr - %3 = llvm.mlir.constant(10 : i32) : i32 - %4 = llvm.mlir.constant(1 : i32) : i32 - // expected-error@below {{not yet implemented: Unhandled clause firstprivate in omp.simd operation}} - // expected-error@below {{LLVM Translation failed for operation: omp.simd}} - omp.simd private(@_QFEp_firstprivate_i32 %1 -> %arg0 : !llvm.ptr) { - omp.loop_nest (%arg2) : i32 = (%4) to (%3) inclusive step (%4) { - omp.yield - } - } - llvm.return -} From 971daec68cf21ded2b5abf6153a8de66edf15948 Mon Sep 17 00:00:00 2001 From: Tom Eccles Date: Thu, 3 Jul 2025 11:08:31 +0000 Subject: [PATCH 3/5] Disallow SIMD FIRSTPRIVATE in the MLIR verifier --- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 19 +++++++++++ mlir/test/Dialect/OpenMP/invalid.mlir | 33 ++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index e94d570b57122..ffc84781f77ff 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -15,11 +15,13 @@ #include "mlir/Dialect/Func/IR/FuncOps.h" #include "mlir/Dialect/LLVMIR/LLVMTypes.h" #include "mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.h" +#include "mlir/Dialect/OpenMP/OpenMPClauseOperands.h" #include "mlir/IR/Attributes.h" #include "mlir/IR/BuiltinAttributes.h" #include "mlir/IR/DialectImplementation.h" #include "mlir/IR/OpImplementation.h" #include "mlir/IR/OperationSupport.h" +#include "mlir/IR/SymbolTable.h" #include "mlir/Interfaces/FoldInterfaces.h" #include "llvm/ADT/ArrayRef.h" @@ -2640,6 +2642,23 @@ LogicalResult SimdOp::verify() { return emitError() << "'omp.composite' attribute present in non-composite wrapper"; + // Firstprivate is not allowed for SIMD in the standard. Check that none of + // the private decls are for firstprivate. + std::optional privateSyms = getPrivateSyms(); + if (privateSyms) { + for (const Attribute &sym : *privateSyms) { + auto symRef = cast(sym); + omp::PrivateClauseOp privatizer = + SymbolTable::lookupNearestSymbolFrom( + getOperation(), symRef); + if (!privatizer) + return emitError() << "Cannot find privatizer '" << symRef << "'"; + if (privatizer.getDataSharingType() == + DataSharingClauseType::FirstPrivate) + return emitError() << "FIRSTPRIVATE cannot be used with SIMD"; + } + } + return success(); } diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir index 060b3cd2455a0..7608ad57c7967 100644 --- a/mlir/test/Dialect/OpenMP/invalid.mlir +++ b/mlir/test/Dialect/OpenMP/invalid.mlir @@ -480,6 +480,39 @@ func.func @omp_simd_pretty_simdlen_safelen(%lb : index, %ub : index, %step : ind // ----- +func.func @omp_simd_bad_privatizer(%lb : index, %ub : index, %step : index) { + %0 = llvm.mlir.constant(1 : i64) : i64 + %1 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr + // expected-error @below {{Cannot find privatizer '@not_defined'}} + omp.simd private(@not_defined %1 -> %arg0 : !llvm.ptr) { + omp.loop_nest (%arg2) : index = (%lb) to (%ub) inclusive step (%step) { + omp.yield + } + } +} + +// ----- + +omp.private {type = firstprivate} @_QFEp_firstprivate_i32 : i32 copy { +^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr): + %0 = llvm.load %arg0 : !llvm.ptr -> i32 + llvm.store %0, %arg1 : i32, !llvm.ptr + omp.yield(%arg1 : !llvm.ptr) +} +func.func @omp_simd_firstprivate(%lb : index, %ub : index, %step : index) { + %0 = llvm.mlir.constant(1 : i64) : i64 + %1 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr + // expected-error @below {{FIRSTPRIVATE cannot be used with SIMD}} + omp.simd private(@_QFEp_firstprivate_i32 %1 -> %arg0 : !llvm.ptr) { + omp.loop_nest (%arg2) : index = (%lb) to (%ub) inclusive step (%step) { + omp.yield + } + } + llvm.return +} + +// ----- + // expected-error @below {{op expects alloc region to yield a value of the reduction type}} omp.declare_reduction @add_f32 : f32 alloc { From 31f60c72ee49557c2b6a434d53a70f05372753df Mon Sep 17 00:00:00 2001 From: Tom Eccles Date: Thu, 3 Jul 2025 12:35:25 +0000 Subject: [PATCH 4/5] Update comment --- .../Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index 7b07243c5f843..a6e0302011386 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -2894,7 +2894,8 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder, .failed()) return failure(); - // TODO: no call to copyFirstPrivateVars? + // No call to copyFirstPrivateVars because FIRSTPRIVATE is not allowed for + // SIMD assert(afterAllocas.get()->getSinglePredecessor()); if (failed(initReductionVars(simdOp, reductionArgs, builder, From ef997406d4051e1e3e4b6c6e32b31dd1647377a3 Mon Sep 17 00:00:00 2001 From: Tom Eccles Date: Fri, 4 Jul 2025 11:14:45 +0000 Subject: [PATCH 5/5] Fix comment --- .../Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp index a6e0302011386..b3eb1ab1832c1 100644 --- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp +++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp @@ -2895,7 +2895,7 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder, return failure(); // No call to copyFirstPrivateVars because FIRSTPRIVATE is not allowed for - // SIMD + // SIMD. assert(afterAllocas.get()->getSinglePredecessor()); if (failed(initReductionVars(simdOp, reductionArgs, builder,