From 70c818b7259372f24a198605eaf709a3d43f4a31 Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Thu, 2 May 2024 13:50:43 -0500 Subject: [PATCH 1/3] [mlir][OpenMP] - Honor dependencies in code-generation of the if clause in omp.task correctly This patch fixes code generation of the if clause specifically when the condition evaluates to false and when the task directive has the depend clause on it. When the if clause of a task construct evaluates to false, then the task is an undeferred task. This undeferred task still has to honor dependencies. Previously, the OpenMPIRbuilder didn't honor dependencies. This patch fixes that. Fixes https://github.com/llvm/llvm-project/issues/90869 --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 9 ++++++++ mlir/test/Target/LLVMIR/omptask_if_false.mlir | 23 +++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 mlir/test/Target/LLVMIR/omptask_if_false.mlir diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 4d2d352f7520b..695308419e0d2 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -1887,6 +1887,15 @@ OpenMPIRBuilder::createTask(const LocationDescription &Loc, SplitBlockAndInsertIfThenElse(IfCondition, IfTerminator, &ThenTI, &ElseTI); Builder.SetInsertPoint(ElseTI); + + if (Dependencies.size()) { + Function *TaskWaitFn = + getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_taskwait_deps_51); + Builder.CreateCall(TaskWaitFn, {Ident, ThreadID, Builder.getInt32(Dependencies.size()), + DepArray, ConstantInt::get(Builder.getInt32Ty(), 0), + ConstantPointerNull::get(PointerType::getUnqual(M.getContext())), + ConstantInt::get(Builder.getInt32Ty(), false)}); + } Function *TaskBeginFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_task_begin_if0); Function *TaskCompleteFn = diff --git a/mlir/test/Target/LLVMIR/omptask_if_false.mlir b/mlir/test/Target/LLVMIR/omptask_if_false.mlir new file mode 100644 index 0000000000000..c7fd5eb62a977 --- /dev/null +++ b/mlir/test/Target/LLVMIR/omptask_if_false.mlir @@ -0,0 +1,23 @@ +// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s + +module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry, dense<32> : vector<4xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry, dense<64> : vector<4xi64>>, #dlti.dl_entry, dense<32> : vector<4xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<4xi64>>, #dlti.dl_entry<"dlti.endianness", "little">, #dlti.dl_entry<"dlti.stack_alignment", 128 : i64>>, fir.defaultkind = "a1c4d8i4l4r4", fir.kindmap = "", llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128", llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_gpu = false, omp.is_target_device = false, omp.version = #omp.version} +{ + llvm.func @foo_(%arg0: !llvm.ptr {fir.bindc_name = "n"}, %arg1: !llvm.ptr {fir.bindc_name = "r"}) attributes {fir.internal_name = "_QPfoo"} { + %0 = llvm.mlir.constant(false) : i1 + omp.task if(%0) depend(taskdependin -> %arg0 : !llvm.ptr) { + %1 = llvm.load %arg0 : !llvm.ptr -> i32 + llvm.store %1, %arg1 : i32, !llvm.ptr + omp.terminator + } + llvm.return + } + llvm.func @llvm.stacksave.p0() -> !llvm.ptr attributes {sym_visibility = "private"} + llvm.func @llvm.stackrestore.p0(!llvm.ptr) attributes {sym_visibility = "private"} +} + + +// CHECK: call void @__kmpc_omp_taskwait_deps_51 +// CHECK-NEXT: call void @__kmpc_omp_task_begin_if0 +// CHECK-NEXT: call void @foo_..omp_par +// CHECK-NEXT: call void @__kmpc_omp_task_complete_if0 + From 09c87f091a77a8331e50904828c3e123e3a0ed2b Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Thu, 2 May 2024 14:00:28 -0500 Subject: [PATCH 2/3] Fix indentation --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 695308419e0d2..7444778c78330 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -1891,10 +1891,12 @@ OpenMPIRBuilder::createTask(const LocationDescription &Loc, if (Dependencies.size()) { Function *TaskWaitFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_taskwait_deps_51); - Builder.CreateCall(TaskWaitFn, {Ident, ThreadID, Builder.getInt32(Dependencies.size()), - DepArray, ConstantInt::get(Builder.getInt32Ty(), 0), - ConstantPointerNull::get(PointerType::getUnqual(M.getContext())), - ConstantInt::get(Builder.getInt32Ty(), false)}); + Builder.CreateCall( + TaskWaitFn, + {Ident, ThreadID, Builder.getInt32(Dependencies.size()), DepArray, + ConstantInt::get(Builder.getInt32Ty(), 0), + ConstantPointerNull::get(PointerType::getUnqual(M.getContext())), + ConstantInt::get(Builder.getInt32Ty(), false)}); } Function *TaskBeginFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_task_begin_if0); From 5c5d976325f77373358c720774156e31e3cb45a2 Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Tue, 7 May 2024 14:21:21 -0500 Subject: [PATCH 3/3] Address review comments --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 8 ++++--- mlir/test/Target/LLVMIR/omptask_if_false.mlir | 22 +++++++------------ 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 7444778c78330..dae3aa1216617 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -1870,6 +1870,9 @@ OpenMPIRBuilder::createTask(const LocationDescription &Loc, // call @__kmpc_omp_task(...) // br label %exit // else: + // ;; Wait for resolution of dependencies, if any, before + // ;; beginning the task + // call @__kmpc_omp_wait_deps(...) // call @__kmpc_omp_task_begin_if0(...) // call @outlined_fn(...) // call @__kmpc_omp_task_complete_if0(...) @@ -1890,13 +1893,12 @@ OpenMPIRBuilder::createTask(const LocationDescription &Loc, if (Dependencies.size()) { Function *TaskWaitFn = - getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_taskwait_deps_51); + getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_wait_deps); Builder.CreateCall( TaskWaitFn, {Ident, ThreadID, Builder.getInt32(Dependencies.size()), DepArray, ConstantInt::get(Builder.getInt32Ty(), 0), - ConstantPointerNull::get(PointerType::getUnqual(M.getContext())), - ConstantInt::get(Builder.getInt32Ty(), false)}); + ConstantPointerNull::get(PointerType::getUnqual(M.getContext()))}); } Function *TaskBeginFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_task_begin_if0); diff --git a/mlir/test/Target/LLVMIR/omptask_if_false.mlir b/mlir/test/Target/LLVMIR/omptask_if_false.mlir index c7fd5eb62a977..c6014a76add6d 100644 --- a/mlir/test/Target/LLVMIR/omptask_if_false.mlir +++ b/mlir/test/Target/LLVMIR/omptask_if_false.mlir @@ -1,22 +1,16 @@ // RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s -module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry, dense<32> : vector<4xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry, dense<64> : vector<4xi64>>, #dlti.dl_entry, dense<32> : vector<4xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<2xi64>>, #dlti.dl_entry : vector<4xi64>>, #dlti.dl_entry<"dlti.endianness", "little">, #dlti.dl_entry<"dlti.stack_alignment", 128 : i64>>, fir.defaultkind = "a1c4d8i4l4r4", fir.kindmap = "", llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128", llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_gpu = false, omp.is_target_device = false, omp.version = #omp.version} -{ - llvm.func @foo_(%arg0: !llvm.ptr {fir.bindc_name = "n"}, %arg1: !llvm.ptr {fir.bindc_name = "r"}) attributes {fir.internal_name = "_QPfoo"} { - %0 = llvm.mlir.constant(false) : i1 - omp.task if(%0) depend(taskdependin -> %arg0 : !llvm.ptr) { - %1 = llvm.load %arg0 : !llvm.ptr -> i32 - llvm.store %1, %arg1 : i32, !llvm.ptr - omp.terminator - } - llvm.return +llvm.func @foo_(%arg0: !llvm.ptr {fir.bindc_name = "n"}, %arg1: !llvm.ptr {fir.bindc_name = "r"}) attributes {fir.internal_name = "_QPfoo"} { + %0 = llvm.mlir.constant(false) : i1 + omp.task if(%0) depend(taskdependin -> %arg0 : !llvm.ptr) { + %1 = llvm.load %arg0 : !llvm.ptr -> i32 + llvm.store %1, %arg1 : i32, !llvm.ptr + omp.terminator } - llvm.func @llvm.stacksave.p0() -> !llvm.ptr attributes {sym_visibility = "private"} - llvm.func @llvm.stackrestore.p0(!llvm.ptr) attributes {sym_visibility = "private"} + llvm.return } - -// CHECK: call void @__kmpc_omp_taskwait_deps_51 +// CHECK: call void @__kmpc_omp_wait_deps // CHECK-NEXT: call void @__kmpc_omp_task_begin_if0 // CHECK-NEXT: call void @foo_..omp_par // CHECK-NEXT: call void @__kmpc_omp_task_complete_if0