From 1bf727775884cd228c71737015374e4596f063af Mon Sep 17 00:00:00 2001 From: Abid Qadeer Date: Thu, 28 Nov 2024 21:27:14 +0000 Subject: [PATCH 1/6] [OMPIRBuilder][debug] Fix debug info for variables in target region. 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. --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 66 +++++++++++++++++++ .../Target/LLVMIR/omptarget-debug-var-1.mlir | 63 ++++++++++++++++++ .../Target/LLVMIR/omptarget-debug-var-2.mlir | 62 +++++++++++++++++ 3 files changed, 191 insertions(+) create mode 100644 mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir create mode 100644 mlir/test/Target/LLVMIR/omptarget-debug-var-2.mlir diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 490a012f69603..79acac0a59dd4 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" @@ -6931,6 +6933,8 @@ static Expected createOutlinedFunction( ? make_range(Func->arg_begin() + 1, Func->arg_end()) : Func->args(); + DenseMap> 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 @@ -6972,6 +6976,7 @@ static Expected 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 @@ -7003,6 +7008,67 @@ static Expected createOutlinedFunction( for (auto Deferred : DeferredReplacement) ReplaceValue(std::get<0>(Deferred), std::get<1>(Deferred), Func); + DenseMap Cache; + SmallDenseMap 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(&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 0000000000000..a20ab130049c6 --- /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 +#real_ty = #llvm.di_basic_type +#file = #llvm.di_file<"target.f90" in ""> +#di_null_type = #llvm.di_null_type +#cu = #llvm.di_compile_unit, + sourceLanguage = DW_LANG_Fortran95, file = #file, isOptimized = false, + emissionKind = Full> +#array_ty = #llvm.di_composite_type> +#sp_ty = #llvm.di_subroutine_type +#g_var = #llvm.di_global_variable +#g_var_expr = #llvm.di_global_variable_expression +#sp = #llvm.di_subprogram, compileUnit = #cu, scope = #file, + name = "test", file = #file, subprogramFlags = "Definition", type = #sp_ty> +#var_arr = #llvm.di_local_variable +#var_i = #llvm.di_local_variable +#var_x = #llvm.di_local_variable + +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 0000000000000..22db86fd85e2c --- /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 +#real_ty = #llvm.di_basic_type +#file = #llvm.di_file<"target.f90" in ""> +#di_null_type = #llvm.di_null_type +#cu = #llvm.di_compile_unit, + sourceLanguage = DW_LANG_Fortran95, file = #file, isOptimized = false, + emissionKind = Full> +#array_ty = #llvm.di_composite_type> +#sp_ty = #llvm.di_subroutine_type +#g_var = #llvm.di_global_variable +#g_var_expr = #llvm.di_global_variable_expression +#sp = #llvm.di_subprogram, compileUnit = #cu, scope = #file, + name = "test", file = #file, subprogramFlags = "Definition", type = #sp_ty> +#var_arr = #llvm.di_local_variable +#var_i = #llvm.di_local_variable +#var_x = #llvm.di_local_variable + +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]]{{.*}}) From 320c1c64a0457c5917b3784fab152dd3b7cdeecb Mon Sep 17 00:00:00 2001 From: Abid Qadeer Date: Wed, 5 Feb 2025 16:15:21 +0000 Subject: [PATCH 2/6] Handle review comments. Move the variable update to a separate helper function. --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 130 ++++++++++-------- .../Target/LLVMIR/omptarget-debug-var-1.mlir | 2 +- 2 files changed, 70 insertions(+), 62 deletions(-) diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 79acac0a59dd4..867e5ffcbf5ff 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -6808,6 +6808,73 @@ FunctionCallee OpenMPIRBuilder::createDispatchDeinitFunction() { return getOrCreateRuntimeFunction(M, omp::OMPRTL___kmpc_dispatch_deinit); } +static void FixupDebugInfoForOutlinedFunction( + OpenMPIRBuilder &OMPBuilder, IRBuilderBase &Builder, Function *Func, + DenseMap> &ValueReplacementMap) { + DenseMap Cache; + SmallDenseMap 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(&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(); + auto M = Builder.GetInsertBlock()->getModule(); + 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())); + } + } +} + static Expected createOutlinedFunction( OpenMPIRBuilder &OMPBuilder, IRBuilderBase &Builder, const OpenMPIRBuilder::TargetKernelDefaultAttrs &DefaultAttrs, @@ -7008,67 +7075,8 @@ static Expected createOutlinedFunction( for (auto Deferred : DeferredReplacement) ReplaceValue(std::get<0>(Deferred), std::get<1>(Deferred), Func); - DenseMap Cache; - SmallDenseMap 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(&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())); - } - } + FixupDebugInfoForOutlinedFunction(OMPBuilder, Builder, Func, + ValueReplacementMap); return Func; } diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir index a20ab130049c6..950f59f3e7ba5 100644 --- a/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir +++ b/mlir/test/Target/LLVMIR/omptarget-debug-var-1.mlir @@ -26,7 +26,7 @@ #var_x = #llvm.di_local_variable -module attributes {omp.is_target_device = true} { +module attributes {llvm.target_triple = "amdgcn-amd-amdhsa", omp.is_target_device = true} { llvm.func @test() { %0 = llvm.mlir.constant(1 : i64) : i64 %1 = llvm.alloca %0 x f32 : (i64) -> !llvm.ptr From d31c08d22534c40922a5ae7f2b96c4c4a0b4b38d Mon Sep 17 00:00:00 2001 From: Abid Qadeer Date: Thu, 6 Feb 2025 15:56:56 +0000 Subject: [PATCH 3/6] Handle review comments. 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. --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 80 +++++++++++------------ 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 867e5ffcbf5ff..a05428be3a898 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -6811,6 +6811,11 @@ FunctionCallee OpenMPIRBuilder::createDispatchDeinitFunction() { static void FixupDebugInfoForOutlinedFunction( OpenMPIRBuilder &OMPBuilder, IRBuilderBase &Builder, Function *Func, DenseMap> &ValueReplacementMap) { + + DISubprogram *NewSP = Func->getSubprogram(); + if (!NewSP) + return; + DenseMap Cache; SmallDenseMap RemappedVariables; @@ -6828,50 +6833,41 @@ static void FixupDebugInfoForOutlinedFunction( 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(&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)); + auto UpdateDebugRecord = [&](auto *DR) { + DILocalVariable *OldVar = DR->getVariable(); + unsigned ArgNo = OldVar->getArg(); + for (auto Loc : DR->location_ops()) { + auto Iter = ValueReplacementMap.find(Loc); + if (Iter != ValueReplacementMap.end()) { + DR->replaceVariableLocationOp(Loc, std::get<0>(Iter->second)); + ArgNo = std::get<1>(Iter->second) + 1; } - 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(); - auto M = Builder.GetInsertBlock()->getModule(); - 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())); } + DR->setVariable(GetUpdatedDIVariable(OldVar, ArgNo)); + }; + + // 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(&I)) + UpdateDebugRecord(DDI); + + for (DbgVariableRecord &DVR : filterDbgVars(I.getDbgRecordRange())) + UpdateDebugRecord(&DVR); + } + // An extra argument is passed to the device. Create the debug data for it. + if (OMPBuilder.Config.isTargetDevice()) { + DICompileUnit *CU = NewSP->getUnit(); + auto M = Builder.GetInsertBlock()->getModule(); + 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())); } } From 4faa1d4bbb1c069c3616641982ec9b250f1bd8f3 Mon Sep 17 00:00:00 2001 From: Abid Qadeer Date: Thu, 6 Feb 2025 17:40:38 +0000 Subject: [PATCH 4/6] Use simpler way to get Module handle. --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index a05428be3a898..4eb8234e9c7df 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -6858,7 +6858,7 @@ static void FixupDebugInfoForOutlinedFunction( // An extra argument is passed to the device. Create the debug data for it. if (OMPBuilder.Config.isTargetDevice()) { DICompileUnit *CU = NewSP->getUnit(); - auto M = Builder.GetInsertBlock()->getModule(); + Module *M = Func->getParent(); DIBuilder DB(*M, true, CU); DIType *VoidPtrTy = DB.createQualifiedType(dwarf::DW_TAG_pointer_type, nullptr); From e214fd6f35b6f3df96e9a2d7c1de5dd9fd9ce782 Mon Sep 17 00:00:00 2001 From: Abid Qadeer Date: Fri, 7 Feb 2025 17:01:09 +0000 Subject: [PATCH 5/6] Improve handling of privatized variables. --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 4eb8234e9c7df..78c465c0a2eb5 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -6822,20 +6822,23 @@ static void FixupDebugInfoForOutlinedFunction( 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()); - } + // Only use cached variable if the arg number matches. This is important + // so that DIVariable created for privatized variables are not discarded. + if (NewVar && (arg == NewVar->getArg())) + return 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; }; auto UpdateDebugRecord = [&](auto *DR) { DILocalVariable *OldVar = DR->getVariable(); - unsigned ArgNo = OldVar->getArg(); + unsigned ArgNo = 0; for (auto Loc : DR->location_ops()) { auto Iter = ValueReplacementMap.find(Loc); if (Iter != ValueReplacementMap.end()) { From 5cc9b9bd0dae156e1de705b5fcc525c8fb490f62 Mon Sep 17 00:00:00 2001 From: Abid Qadeer Date: Fri, 7 Feb 2025 17:26:16 +0000 Subject: [PATCH 6/6] Fix clang-format issue. --- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 78c465c0a2eb5..4dd22b2f313b1 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -6828,7 +6828,7 @@ static void FixupDebugInfoForOutlinedFunction( return NewVar; DILocalScope *NewScope = DILocalScope::cloneScopeForSubprogram( - *OldVar->getScope(), *NewSP, Builder.getContext(), Cache); + *OldVar->getScope(), *NewSP, Builder.getContext(), Cache); NewVar = llvm::DILocalVariable::get( Builder.getContext(), NewScope, OldVar->getName(), OldVar->getFile(), OldVar->getLine(), OldVar->getType(), arg, OldVar->getFlags(),