[OMPIRBuilder][debug] Fix debug info for variables in target region. #118314
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abidh
requested review from
Meinersbur,
skatrak,
tblah,
kiranchandramohan,
jdoerfert,
agozillon,
dpalermo and
kiranktp
llvmbot
added
mlir:llvm
mlir
flang:openmp
clang:openmp
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: Abid Qadeer (abidh) ChangesWhen a new function is created to handle OpenMP target region, the variables used in it are passed as arguments. The scope and the location of these variable contains still point to te parent function of the target region. Such variables will fail in Verifier as the scope of the variables will be different from the containing functions. Currently, flang is the only user of createOutlinedFunction and it does not generate any debug data for the the variables in the target region to avoid this error. When this PR is in, we should be able to remove this limit in the flang (and anyother client) and have the better debug experience for the target region. This PR changes the location and scope of the variables in the target region to point to correct entities. It is similar to what fixupDebugInfoPostExtraction does for CodeExtractor. I initially tried to re-use that function but found quickly that it would require quite a bit of re-factoring and additions before it could be used. It was much simpler to make the changes locally. Full diff: https://github.com/llvm/llvm-project/pull/118314.diff 3 Files Affected:
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 1fae138b449ed5..60f63b6c3dda47 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -38,6 +38,8 @@
#include "llvm/IR/Function.h"
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/InstIterator.h"
+#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/MDBuilder.h"
#include "llvm/IR/Metadata.h"
@@ -6830,6 +6832,8 @@ static Expected<Function *> createOutlinedFunction(
? make_range(Func->arg_begin() + 1, Func->arg_end())
: Func->args();
+ DenseMap<Value *, std::tuple<Value *, unsigned>> ValueReplacementMap;
+
auto ReplaceValue = [](Value *Input, Value *InputCopy, Function *Func) {
// Things like GEP's can come in the form of Constants. Constants and
// ConstantExpr's do not have access to the knowledge of what they're
@@ -6871,6 +6875,7 @@ static Expected<Function *> createOutlinedFunction(
if (!AfterIP)
return AfterIP.takeError();
Builder.restoreIP(*AfterIP);
+ ValueReplacementMap[Input] = std::make_tuple(InputCopy, Arg.getArgNo());
// In certain cases a Global may be set up for replacement, however, this
// Global may be used in multiple arguments to the kernel, just segmented
@@ -6902,6 +6907,67 @@ static Expected<Function *> createOutlinedFunction(
for (auto Deferred : DeferredReplacement)
ReplaceValue(std::get<0>(Deferred), std::get<1>(Deferred), Func);
+ DenseMap<const MDNode *, MDNode *> Cache;
+ SmallDenseMap<DILocalVariable *, DILocalVariable *> RemappedVariables;
+
+ auto GetUpdatedDIVariable = [&](DILocalVariable *OldVar, unsigned arg) {
+ auto NewSP = Func->getSubprogram();
+ DILocalVariable *&NewVar = RemappedVariables[OldVar];
+ if (!NewVar) {
+ DILocalScope *NewScope = DILocalScope::cloneScopeForSubprogram(
+ *OldVar->getScope(), *NewSP, Builder.getContext(), Cache);
+ NewVar = llvm::DILocalVariable::get(
+ Builder.getContext(), NewScope, OldVar->getName(), OldVar->getFile(),
+ OldVar->getLine(), OldVar->getType(), arg, OldVar->getFlags(),
+ OldVar->getAlignInBits(), OldVar->getAnnotations());
+ }
+ return NewVar;
+ };
+
+ DISubprogram *NewSP = Func->getSubprogram();
+ if (NewSP) {
+ // The location and scope of variable intrinsics and records still point to
+ // the parent function of the target region. Update them.
+ for (Instruction &I : instructions(Func)) {
+ if (auto *DDI = dyn_cast<llvm::DbgVariableIntrinsic>(&I)) {
+ DILocalVariable *OldVar = DDI->getVariable();
+ unsigned ArgNo = OldVar->getArg();
+ for (auto Loc : DDI->location_ops()) {
+ auto Iter = ValueReplacementMap.find(Loc);
+ if (Iter != ValueReplacementMap.end()) {
+ DDI->replaceVariableLocationOp(Loc, std::get<0>(Iter->second));
+ ArgNo = std::get<1>(Iter->second) + 1;
+ }
+ }
+ DDI->setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
+ }
+ for (DbgVariableRecord &DVR : filterDbgVars(I.getDbgRecordRange())) {
+ DILocalVariable *OldVar = DVR.getVariable();
+ unsigned ArgNo = OldVar->getArg();
+ for (auto Loc : DVR.location_ops()) {
+ auto Iter = ValueReplacementMap.find(Loc);
+ if (Iter != ValueReplacementMap.end()) {
+ DVR.replaceVariableLocationOp(Loc, std::get<0>(Iter->second));
+ ArgNo = std::get<1>(Iter->second) + 1;
+ }
+ }
+ DVR.setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
+ }
+ }
+ // An extra argument is passed to the device. Create the debug data for it.
+ if (OMPBuilder.Config.isTargetDevice()) {
+ DICompileUnit *CU = NewSP->getUnit();
+ DIBuilder DB(*M, true, CU);
+ DIType *VoidPtrTy =
+ DB.createQualifiedType(dwarf::DW_TAG_pointer_type, nullptr);
+ DILocalVariable *Var = DB.createParameterVariable(
+ NewSP, "dyn_ptr", /*ArgNo*/ 1, NewSP->getFile(), /*LineNo=*/0,
+ VoidPtrTy, /*AlwaysPreserve=*/false, DINode::DIFlags::FlagArtificial);
+ auto Loc = DILocation::get(Func->getContext(), 0, 0, NewSP, 0);
+ DB.insertDeclare(&(*Func->arg_begin()), Var, DB.createExpression(), Loc,
+ &(*Func->begin()));
+ }
+ }
return Func;
}
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir
new file mode 100644
index 00000000000000..a20ab130049c6d
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir
@@ -0,0 +1,63 @@
+// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
+
+#int_ty = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer",
+ sizeInBits = 32, encoding = DW_ATE_signed>
+#real_ty = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "real",
+ sizeInBits = 32, encoding = DW_ATE_float>
+#file = #llvm.di_file<"target.f90" in "">
+#di_null_type = #llvm.di_null_type
+#cu = #llvm.di_compile_unit<id = distinct[0]<>,
+ sourceLanguage = DW_LANG_Fortran95, file = #file, isOptimized = false,
+ emissionKind = Full>
+#array_ty = #llvm.di_composite_type<tag = DW_TAG_array_type,
+ baseType = #int_ty, elements = #llvm.di_subrange<count = 10 : i64>>
+#sp_ty = #llvm.di_subroutine_type<callingConvention = DW_CC_program,
+ types = #di_null_type>
+#g_var = #llvm.di_global_variable<scope = #cu, name = "arr",
+ linkageName = "_QFEarr", file = #file, line = 4,
+ type = #array_ty, isDefined = true>
+#g_var_expr = #llvm.di_global_variable_expression<var = #g_var>
+#sp = #llvm.di_subprogram<id = distinct[2]<>, compileUnit = #cu, scope = #file,
+ name = "test", file = #file, subprogramFlags = "Definition", type = #sp_ty>
+#var_arr = #llvm.di_local_variable<scope = #sp,
+ name = "arr", file = #file, line = 4, type = #array_ty>
+#var_i = #llvm.di_local_variable<scope = #sp,
+ name = "i", file = #file, line = 13, type = #int_ty>
+#var_x = #llvm.di_local_variable<scope = #sp,
+ name = "x", file = #file, line = 12, type = #real_ty>
+
+module attributes {omp.is_target_device = true} {
+ llvm.func @test() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x f32 : (i64) -> !llvm.ptr
+ %4 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr
+ %6 = llvm.mlir.constant(9 : index) : i64
+ %7 = llvm.mlir.constant(0 : index) : i64
+ %8 = llvm.mlir.constant(1 : index) : i64
+ %10 = llvm.mlir.constant(10 : index) : i64
+ %11 = llvm.mlir.addressof @_QFEarr : !llvm.ptr
+ %14 = omp.map.info var_ptr(%1 : !llvm.ptr, f32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr
+ %15 = omp.map.bounds lower_bound(%7 : i64) upper_bound(%6 : i64) extent(%10 : i64) stride(%8 : i64) start_idx(%8 : i64)
+ %16 = omp.map.info var_ptr(%11 : !llvm.ptr, !llvm.array<10 x i32>) map_clauses(tofrom) capture(ByRef) bounds(%15) -> !llvm.ptr
+ %17 = omp.map.info var_ptr(%4 : !llvm.ptr, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !llvm.ptr
+ omp.target map_entries(%14 -> %arg0, %16 -> %arg1, %17 -> %arg2 : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
+ llvm.intr.dbg.declare #var_x = %arg0 : !llvm.ptr
+ llvm.intr.dbg.declare #var_arr = %arg1 : !llvm.ptr
+ llvm.intr.dbg.declare #var_i = %arg2 : !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+ } loc(#loc3)
+ llvm.mlir.global internal @_QFEarr() {addr_space = 0 : i32, dbg_exprs = [#g_var_expr]} : !llvm.array<10 x i32> {
+ } loc(#loc4)
+}
+#loc1 = loc("target.f90":4:7)
+#loc2 = loc("target.f90":11:7)
+#loc3 = loc(fused<#sp>[#loc2])
+#loc4 = loc(fused<#g_var>[#loc1])
+
+// CHECK: ![[SP:[0-9]+]] = distinct !DISubprogram(name: "__omp_offloading{{.*}}test{{.*}})
+// CHECK: !DILocalVariable(name: "dyn_ptr", arg: 1, scope: ![[SP]]{{.*}}flags: DIFlagArtificial)
+// CHECK: !DILocalVariable(name: "x", arg: 2, scope: ![[SP]]{{.*}})
+// CHECK: !DILocalVariable(name: "arr", arg: 3, scope: ![[SP]]{{.*}})
+// CHECK: !DILocalVariable(name: "i", arg: 4, scope: ![[SP]]{{.*}})
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-var-2.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-var-2.mlir
new file mode 100644
index 00000000000000..22db86fd85e2cc
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/omptarget-debug-var-2.mlir
@@ -0,0 +1,62 @@
+// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
+
+#int_ty = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer",
+ sizeInBits = 32, encoding = DW_ATE_signed>
+#real_ty = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "real",
+ sizeInBits = 32, encoding = DW_ATE_float>
+#file = #llvm.di_file<"target.f90" in "">
+#di_null_type = #llvm.di_null_type
+#cu = #llvm.di_compile_unit<id = distinct[0]<>,
+ sourceLanguage = DW_LANG_Fortran95, file = #file, isOptimized = false,
+ emissionKind = Full>
+#array_ty = #llvm.di_composite_type<tag = DW_TAG_array_type,
+ baseType = #int_ty, elements = #llvm.di_subrange<count = 10 : i64>>
+#sp_ty = #llvm.di_subroutine_type<callingConvention = DW_CC_program,
+ types = #di_null_type>
+#g_var = #llvm.di_global_variable<scope = #cu, name = "arr",
+ linkageName = "_QFEarr", file = #file, line = 4,
+ type = #array_ty, isDefined = true>
+#g_var_expr = #llvm.di_global_variable_expression<var = #g_var>
+#sp = #llvm.di_subprogram<id = distinct[2]<>, compileUnit = #cu, scope = #file,
+ name = "test", file = #file, subprogramFlags = "Definition", type = #sp_ty>
+#var_arr = #llvm.di_local_variable<scope = #sp,
+ name = "arr", file = #file, line = 4, type = #array_ty>
+#var_i = #llvm.di_local_variable<scope = #sp,
+ name = "i", file = #file, line = 13, type = #int_ty>
+#var_x = #llvm.di_local_variable<scope = #sp,
+ name = "x", file = #file, line = 12, type = #real_ty>
+
+module attributes {omp.is_target_device = false} {
+ llvm.func @test() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x f32 : (i64) -> !llvm.ptr
+ %4 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr
+ %6 = llvm.mlir.constant(9 : index) : i64
+ %7 = llvm.mlir.constant(0 : index) : i64
+ %8 = llvm.mlir.constant(1 : index) : i64
+ %10 = llvm.mlir.constant(10 : index) : i64
+ %11 = llvm.mlir.addressof @_QFEarr : !llvm.ptr
+ %14 = omp.map.info var_ptr(%1 : !llvm.ptr, f32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr
+ %15 = omp.map.bounds lower_bound(%7 : i64) upper_bound(%6 : i64) extent(%10 : i64) stride(%8 : i64) start_idx(%8 : i64)
+ %16 = omp.map.info var_ptr(%11 : !llvm.ptr, !llvm.array<10 x i32>) map_clauses(tofrom) capture(ByRef) bounds(%15) -> !llvm.ptr
+ %17 = omp.map.info var_ptr(%4 : !llvm.ptr, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !llvm.ptr
+ omp.target map_entries(%14 -> %arg0, %16 -> %arg1, %17 -> %arg2 : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
+ llvm.intr.dbg.declare #var_x = %arg0 : !llvm.ptr
+ llvm.intr.dbg.declare #var_arr = %arg1 : !llvm.ptr
+ llvm.intr.dbg.declare #var_i = %arg2 : !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+ } loc(#loc3)
+ llvm.mlir.global internal @_QFEarr() {addr_space = 0 : i32, dbg_exprs = [#g_var_expr]} : !llvm.array<10 x i32> {
+ } loc(#loc4)
+}
+#loc1 = loc("target.f90":4:7)
+#loc2 = loc("target.f90":11:7)
+#loc3 = loc(fused<#sp>[#loc2])
+#loc4 = loc(fused<#g_var>[#loc1])
+
+// CHECK: ![[SP:[0-9]+]] = distinct !DISubprogram(name: "__omp_offloading{{.*}}test{{.*}})
+// CHECK: !DILocalVariable(name: "x", arg: 1, scope: ![[SP]]{{.*}})
+// CHECK: !DILocalVariable(name: "arr", arg: 2, scope: ![[SP]]{{.*}})
+// CHECK: !DILocalVariable(name: "i", arg: 3, scope: ![[SP]]{{.*}})
|
|
@llvm/pr-subscribers-flang-openmp Author: Abid Qadeer (abidh) ChangesWhen a new function is created to handle OpenMP target region, the variables used in it are passed as arguments. The scope and the location of these variable contains still point to te parent function of the target region. Such variables will fail in Verifier as the scope of the variables will be different from the containing functions. Currently, flang is the only user of createOutlinedFunction and it does not generate any debug data for the the variables in the target region to avoid this error. When this PR is in, we should be able to remove this limit in the flang (and anyother client) and have the better debug experience for the target region. This PR changes the location and scope of the variables in the target region to point to correct entities. It is similar to what fixupDebugInfoPostExtraction does for CodeExtractor. I initially tried to re-use that function but found quickly that it would require quite a bit of re-factoring and additions before it could be used. It was much simpler to make the changes locally. Full diff: https://github.com/llvm/llvm-project/pull/118314.diff 3 Files Affected:
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 1fae138b449ed5..60f63b6c3dda47 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -38,6 +38,8 @@
#include "llvm/IR/Function.h"
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/InstIterator.h"
+#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/MDBuilder.h"
#include "llvm/IR/Metadata.h"
@@ -6830,6 +6832,8 @@ static Expected<Function *> createOutlinedFunction(
? make_range(Func->arg_begin() + 1, Func->arg_end())
: Func->args();
+ DenseMap<Value *, std::tuple<Value *, unsigned>> ValueReplacementMap;
+
auto ReplaceValue = [](Value *Input, Value *InputCopy, Function *Func) {
// Things like GEP's can come in the form of Constants. Constants and
// ConstantExpr's do not have access to the knowledge of what they're
@@ -6871,6 +6875,7 @@ static Expected<Function *> createOutlinedFunction(
if (!AfterIP)
return AfterIP.takeError();
Builder.restoreIP(*AfterIP);
+ ValueReplacementMap[Input] = std::make_tuple(InputCopy, Arg.getArgNo());
// In certain cases a Global may be set up for replacement, however, this
// Global may be used in multiple arguments to the kernel, just segmented
@@ -6902,6 +6907,67 @@ static Expected<Function *> createOutlinedFunction(
for (auto Deferred : DeferredReplacement)
ReplaceValue(std::get<0>(Deferred), std::get<1>(Deferred), Func);
+ DenseMap<const MDNode *, MDNode *> Cache;
+ SmallDenseMap<DILocalVariable *, DILocalVariable *> RemappedVariables;
+
+ auto GetUpdatedDIVariable = [&](DILocalVariable *OldVar, unsigned arg) {
+ auto NewSP = Func->getSubprogram();
+ DILocalVariable *&NewVar = RemappedVariables[OldVar];
+ if (!NewVar) {
+ DILocalScope *NewScope = DILocalScope::cloneScopeForSubprogram(
+ *OldVar->getScope(), *NewSP, Builder.getContext(), Cache);
+ NewVar = llvm::DILocalVariable::get(
+ Builder.getContext(), NewScope, OldVar->getName(), OldVar->getFile(),
+ OldVar->getLine(), OldVar->getType(), arg, OldVar->getFlags(),
+ OldVar->getAlignInBits(), OldVar->getAnnotations());
+ }
+ return NewVar;
+ };
+
+ DISubprogram *NewSP = Func->getSubprogram();
+ if (NewSP) {
+ // The location and scope of variable intrinsics and records still point to
+ // the parent function of the target region. Update them.
+ for (Instruction &I : instructions(Func)) {
+ if (auto *DDI = dyn_cast<llvm::DbgVariableIntrinsic>(&I)) {
+ DILocalVariable *OldVar = DDI->getVariable();
+ unsigned ArgNo = OldVar->getArg();
+ for (auto Loc : DDI->location_ops()) {
+ auto Iter = ValueReplacementMap.find(Loc);
+ if (Iter != ValueReplacementMap.end()) {
+ DDI->replaceVariableLocationOp(Loc, std::get<0>(Iter->second));
+ ArgNo = std::get<1>(Iter->second) + 1;
+ }
+ }
+ DDI->setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
+ }
+ for (DbgVariableRecord &DVR : filterDbgVars(I.getDbgRecordRange())) {
+ DILocalVariable *OldVar = DVR.getVariable();
+ unsigned ArgNo = OldVar->getArg();
+ for (auto Loc : DVR.location_ops()) {
+ auto Iter = ValueReplacementMap.find(Loc);
+ if (Iter != ValueReplacementMap.end()) {
+ DVR.replaceVariableLocationOp(Loc, std::get<0>(Iter->second));
+ ArgNo = std::get<1>(Iter->second) + 1;
+ }
+ }
+ DVR.setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
+ }
+ }
+ // An extra argument is passed to the device. Create the debug data for it.
+ if (OMPBuilder.Config.isTargetDevice()) {
+ DICompileUnit *CU = NewSP->getUnit();
+ DIBuilder DB(*M, true, CU);
+ DIType *VoidPtrTy =
+ DB.createQualifiedType(dwarf::DW_TAG_pointer_type, nullptr);
+ DILocalVariable *Var = DB.createParameterVariable(
+ NewSP, "dyn_ptr", /*ArgNo*/ 1, NewSP->getFile(), /*LineNo=*/0,
+ VoidPtrTy, /*AlwaysPreserve=*/false, DINode::DIFlags::FlagArtificial);
+ auto Loc = DILocation::get(Func->getContext(), 0, 0, NewSP, 0);
+ DB.insertDeclare(&(*Func->arg_begin()), Var, DB.createExpression(), Loc,
+ &(*Func->begin()));
+ }
+ }
return Func;
}
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir
new file mode 100644
index 00000000000000..a20ab130049c6d
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir
@@ -0,0 +1,63 @@
+// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
+
+#int_ty = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer",
+ sizeInBits = 32, encoding = DW_ATE_signed>
+#real_ty = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "real",
+ sizeInBits = 32, encoding = DW_ATE_float>
+#file = #llvm.di_file<"target.f90" in "">
+#di_null_type = #llvm.di_null_type
+#cu = #llvm.di_compile_unit<id = distinct[0]<>,
+ sourceLanguage = DW_LANG_Fortran95, file = #file, isOptimized = false,
+ emissionKind = Full>
+#array_ty = #llvm.di_composite_type<tag = DW_TAG_array_type,
+ baseType = #int_ty, elements = #llvm.di_subrange<count = 10 : i64>>
+#sp_ty = #llvm.di_subroutine_type<callingConvention = DW_CC_program,
+ types = #di_null_type>
+#g_var = #llvm.di_global_variable<scope = #cu, name = "arr",
+ linkageName = "_QFEarr", file = #file, line = 4,
+ type = #array_ty, isDefined = true>
+#g_var_expr = #llvm.di_global_variable_expression<var = #g_var>
+#sp = #llvm.di_subprogram<id = distinct[2]<>, compileUnit = #cu, scope = #file,
+ name = "test", file = #file, subprogramFlags = "Definition", type = #sp_ty>
+#var_arr = #llvm.di_local_variable<scope = #sp,
+ name = "arr", file = #file, line = 4, type = #array_ty>
+#var_i = #llvm.di_local_variable<scope = #sp,
+ name = "i", file = #file, line = 13, type = #int_ty>
+#var_x = #llvm.di_local_variable<scope = #sp,
+ name = "x", file = #file, line = 12, type = #real_ty>
+
+module attributes {omp.is_target_device = true} {
+ llvm.func @test() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x f32 : (i64) -> !llvm.ptr
+ %4 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr
+ %6 = llvm.mlir.constant(9 : index) : i64
+ %7 = llvm.mlir.constant(0 : index) : i64
+ %8 = llvm.mlir.constant(1 : index) : i64
+ %10 = llvm.mlir.constant(10 : index) : i64
+ %11 = llvm.mlir.addressof @_QFEarr : !llvm.ptr
+ %14 = omp.map.info var_ptr(%1 : !llvm.ptr, f32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr
+ %15 = omp.map.bounds lower_bound(%7 : i64) upper_bound(%6 : i64) extent(%10 : i64) stride(%8 : i64) start_idx(%8 : i64)
+ %16 = omp.map.info var_ptr(%11 : !llvm.ptr, !llvm.array<10 x i32>) map_clauses(tofrom) capture(ByRef) bounds(%15) -> !llvm.ptr
+ %17 = omp.map.info var_ptr(%4 : !llvm.ptr, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !llvm.ptr
+ omp.target map_entries(%14 -> %arg0, %16 -> %arg1, %17 -> %arg2 : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
+ llvm.intr.dbg.declare #var_x = %arg0 : !llvm.ptr
+ llvm.intr.dbg.declare #var_arr = %arg1 : !llvm.ptr
+ llvm.intr.dbg.declare #var_i = %arg2 : !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+ } loc(#loc3)
+ llvm.mlir.global internal @_QFEarr() {addr_space = 0 : i32, dbg_exprs = [#g_var_expr]} : !llvm.array<10 x i32> {
+ } loc(#loc4)
+}
+#loc1 = loc("target.f90":4:7)
+#loc2 = loc("target.f90":11:7)
+#loc3 = loc(fused<#sp>[#loc2])
+#loc4 = loc(fused<#g_var>[#loc1])
+
+// CHECK: ![[SP:[0-9]+]] = distinct !DISubprogram(name: "__omp_offloading{{.*}}test{{.*}})
+// CHECK: !DILocalVariable(name: "dyn_ptr", arg: 1, scope: ![[SP]]{{.*}}flags: DIFlagArtificial)
+// CHECK: !DILocalVariable(name: "x", arg: 2, scope: ![[SP]]{{.*}})
+// CHECK: !DILocalVariable(name: "arr", arg: 3, scope: ![[SP]]{{.*}})
+// CHECK: !DILocalVariable(name: "i", arg: 4, scope: ![[SP]]{{.*}})
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-var-2.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-var-2.mlir
new file mode 100644
index 00000000000000..22db86fd85e2cc
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/omptarget-debug-var-2.mlir
@@ -0,0 +1,62 @@
+// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
+
+#int_ty = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer",
+ sizeInBits = 32, encoding = DW_ATE_signed>
+#real_ty = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "real",
+ sizeInBits = 32, encoding = DW_ATE_float>
+#file = #llvm.di_file<"target.f90" in "">
+#di_null_type = #llvm.di_null_type
+#cu = #llvm.di_compile_unit<id = distinct[0]<>,
+ sourceLanguage = DW_LANG_Fortran95, file = #file, isOptimized = false,
+ emissionKind = Full>
+#array_ty = #llvm.di_composite_type<tag = DW_TAG_array_type,
+ baseType = #int_ty, elements = #llvm.di_subrange<count = 10 : i64>>
+#sp_ty = #llvm.di_subroutine_type<callingConvention = DW_CC_program,
+ types = #di_null_type>
+#g_var = #llvm.di_global_variable<scope = #cu, name = "arr",
+ linkageName = "_QFEarr", file = #file, line = 4,
+ type = #array_ty, isDefined = true>
+#g_var_expr = #llvm.di_global_variable_expression<var = #g_var>
+#sp = #llvm.di_subprogram<id = distinct[2]<>, compileUnit = #cu, scope = #file,
+ name = "test", file = #file, subprogramFlags = "Definition", type = #sp_ty>
+#var_arr = #llvm.di_local_variable<scope = #sp,
+ name = "arr", file = #file, line = 4, type = #array_ty>
+#var_i = #llvm.di_local_variable<scope = #sp,
+ name = "i", file = #file, line = 13, type = #int_ty>
+#var_x = #llvm.di_local_variable<scope = #sp,
+ name = "x", file = #file, line = 12, type = #real_ty>
+
+module attributes {omp.is_target_device = false} {
+ llvm.func @test() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x f32 : (i64) -> !llvm.ptr
+ %4 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr
+ %6 = llvm.mlir.constant(9 : index) : i64
+ %7 = llvm.mlir.constant(0 : index) : i64
+ %8 = llvm.mlir.constant(1 : index) : i64
+ %10 = llvm.mlir.constant(10 : index) : i64
+ %11 = llvm.mlir.addressof @_QFEarr : !llvm.ptr
+ %14 = omp.map.info var_ptr(%1 : !llvm.ptr, f32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr
+ %15 = omp.map.bounds lower_bound(%7 : i64) upper_bound(%6 : i64) extent(%10 : i64) stride(%8 : i64) start_idx(%8 : i64)
+ %16 = omp.map.info var_ptr(%11 : !llvm.ptr, !llvm.array<10 x i32>) map_clauses(tofrom) capture(ByRef) bounds(%15) -> !llvm.ptr
+ %17 = omp.map.info var_ptr(%4 : !llvm.ptr, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !llvm.ptr
+ omp.target map_entries(%14 -> %arg0, %16 -> %arg1, %17 -> %arg2 : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
+ llvm.intr.dbg.declare #var_x = %arg0 : !llvm.ptr
+ llvm.intr.dbg.declare #var_arr = %arg1 : !llvm.ptr
+ llvm.intr.dbg.declare #var_i = %arg2 : !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+ } loc(#loc3)
+ llvm.mlir.global internal @_QFEarr() {addr_space = 0 : i32, dbg_exprs = [#g_var_expr]} : !llvm.array<10 x i32> {
+ } loc(#loc4)
+}
+#loc1 = loc("target.f90":4:7)
+#loc2 = loc("target.f90":11:7)
+#loc3 = loc(fused<#sp>[#loc2])
+#loc4 = loc(fused<#g_var>[#loc1])
+
+// CHECK: ![[SP:[0-9]+]] = distinct !DISubprogram(name: "__omp_offloading{{.*}}test{{.*}})
+// CHECK: !DILocalVariable(name: "x", arg: 1, scope: ![[SP]]{{.*}})
+// CHECK: !DILocalVariable(name: "arr", arg: 2, scope: ![[SP]]{{.*}})
+// CHECK: !DILocalVariable(name: "i", arg: 3, scope: ![[SP]]{{.*}})
|
Dinistro
reviewed
Dec 2, 2024
There was a problem hiding this comment.
Is there a particular reason to not combine both tests into one file and use --split-input-file
There was a problem hiding this comment.
I would argue that this should be investigated. Working with splits to group similar tests into the same files has shown to be very valuable.
There was a problem hiding this comment.
This is a current limitation also for clang (#62099). The issue is that it tries to use the name of the file to produce the outlined function's name for the target region, and it has no fallback.
|
Polite ping. |
abidh
added a commit
to abidh/llvm-project
that referenced
this pull request
Jan 14, 2025
|
Ping. I would appreciate if someone can spare some time to review this. |
|
Testing for LLVM components should be done in |
Hi @Meinersbur Thanks for taking a look. I see that we routinely add test in I will investigate if this scenario could be created in a Please let me know if the changes otherwise look reasonable. |
abidh
added a commit
to abidh/llvm-project
that referenced
this pull request
Feb 4, 2025
We currently drop the `DeclareOp` which are not in the entry block of the function. This was done to side step a problem with the variables in OpenMP target region. I have opened llvm#118314 to address the original issue. Once that PR is merged, we can lift this restriction as well.
abidh
added a commit
to abidh/llvm-project
that referenced
this pull request
Feb 4, 2025
jsjodin
reviewed
Feb 4, 2025
| @@ -6902,6 +6907,67 @@ static Expected<Function *> createOutlinedFunction( | |||
| for (auto Deferred : DeferredReplacement) | |||
| ReplaceValue(std::get<0>(Deferred), std::get<1>(Deferred), Func); | |||
|
|
|||
| DenseMap<const MDNode *, MDNode *> Cache; | |||
There was a problem hiding this comment.
All this code looks pretty self-contained. Can this be placed in a helper function instead?
There was a problem hiding this comment.
Thanks for reviewing it. I have moved the code to a separate helper function.
When a new function is created to handle OpenMP target region, the variables used in it are passed as arguments. The scope and the location of these variable contains still point to te parent function of the target region. Such variables will fail in Verifier as the scope of the variables will be different from the containing functions. Currently, flang is the only user of createOutlinedFunction and it does not generate any debug data for the the variables in the target region to avoid this error. When this PR is in, we should be able to remove this limit in the flang (and anyother client) and have the better debug experience for the target region. This PR changes the location and scope of the variables in the target region to point to correct entities. It is similar to what fixupDebugInfoPostExtraction does for CodeExtractor. I initially tried to re-use that function but found quickly that it would require quite a bit of re-factoring and additions before it could be used. It was much simpler to make the changes locally.
Move the variable update to a separate helper function.
There was code duplicated to handle DbgVariableRecord and DbgVariableIntrinsic. That duplication has been removed. Also checked if there is valid DISubprogram right at the start of the function.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
abidh
added a commit
to abidh/llvm-project
that referenced
this pull request
Feb 7, 2025
abidh
added a commit
to abidh/llvm-project
that referenced
this pull request
Feb 10, 2025
We currently drop the `DeclareOp` which are not in the entry block of the function. This was done to side step a problem with the variables in OpenMP target region. I have opened llvm#118314 to address the original issue. Once that PR is merged, we can lift this restriction as well.
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…lvm#118314) When a new function is created to handle OpenMP target region, the variables used in it are passed as arguments. The scope and the location of these variable contains still point to te parent function of the target region. Such variables will fail in Verifier as the scope of the variables will be different from the containing functions. Currently, flang is the only user of createOutlinedFunction and it does not generate any debug data for the the variables in the target region to avoid this error. When this PR is in, we should be able to remove this limit in the flang (and anyother client) and have the better debug experience for the target region. This PR changes the location and scope of the variables in the target region to point to correct entities. It is similar to what fixupDebugInfoPostExtraction does for CodeExtractor. I initially tried to re-use that function but found quickly that it would require quite a bit of re-factoring and additions before it could be used. It was much simpler to make the changes locally.
abidh
added a commit
that referenced
this pull request
Feb 13, 2025
We currently drop the `DeclareOp` which are not in the entry block of the function. This was done to side step a problem with the variables in OpenMP target region. Now that issue has been addressed in #118314 so we can lift this restriction as well.
flovent
pushed a commit
to flovent/llvm-project
that referenced
this pull request
Feb 13, 2025
We currently drop the `DeclareOp` which are not in the entry block of the function. This was done to side step a problem with the variables in OpenMP target region. Now that issue has been addressed in llvm#118314 so we can lift this restriction as well.
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
…lvm#118314) When a new function is created to handle OpenMP target region, the variables used in it are passed as arguments. The scope and the location of these variable contains still point to te parent function of the target region. Such variables will fail in Verifier as the scope of the variables will be different from the containing functions. Currently, flang is the only user of createOutlinedFunction and it does not generate any debug data for the the variables in the target region to avoid this error. When this PR is in, we should be able to remove this limit in the flang (and anyother client) and have the better debug experience for the target region. This PR changes the location and scope of the variables in the target region to point to correct entities. It is similar to what fixupDebugInfoPostExtraction does for CodeExtractor. I initially tried to re-use that function but found quickly that it would require quite a bit of re-factoring and additions before it could be used. It was much simpler to make the changes locally.
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
We currently drop the `DeclareOp` which are not in the entry block of the function. This was done to side step a problem with the variables in OpenMP target region. Now that issue has been addressed in llvm#118314 so we can lift this restriction as well.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…lvm#118314) When a new function is created to handle OpenMP target region, the variables used in it are passed as arguments. The scope and the location of these variable contains still point to te parent function of the target region. Such variables will fail in Verifier as the scope of the variables will be different from the containing functions. Currently, flang is the only user of createOutlinedFunction and it does not generate any debug data for the the variables in the target region to avoid this error. When this PR is in, we should be able to remove this limit in the flang (and anyother client) and have the better debug experience for the target region. This PR changes the location and scope of the variables in the target region to point to correct entities. It is similar to what fixupDebugInfoPostExtraction does for CodeExtractor. I initially tried to re-use that function but found quickly that it would require quite a bit of re-factoring and additions before it could be used. It was much simpler to make the changes locally.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
We currently drop the `DeclareOp` which are not in the entry block of the function. This was done to side step a problem with the variables in OpenMP target region. Now that issue has been addressed in llvm#118314 so we can lift this restriction as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a new function is created to handle OpenMP target region, the variables used in it are passed as arguments. The scope and the location of these variable contains still point to te parent function of the target region. Such variables will fail in Verifier as the scope of the variables will be different from the containing functions.
Currently, flang is the only user of createOutlinedFunction and it does not generate any debug data for the the variables in the target region to avoid this error. When this PR is in, we should be able to remove this limit in the flang (and anyother client) and have the better debug experience for the target region.
This PR changes the location and scope of the variables in the target region to point to correct entities. It is similar to what fixupDebugInfoPostExtraction does for CodeExtractor. I initially tried to re-use that function but found quickly that it would require quite a bit of re-factoring and additions before it could be used. It was much simpler to make the changes locally.