[flang][debug] Generate DISubprogramAttr for omp::TargetOp. #146532
Merged
+311
−58
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
There are DeclareOp present for the variables mapped into target region. That allow us to generate debug information for them. Bu the TargetOp is still part of parent function and those variables get the parent function's DISubprogram as a scope. In OMPIRBuilder, a new function is created for the TargetOp. We also create a new DISubprogram for it. All the variables that were in the target region now have to be updated to have the correct scope. This after the fact updating of debug information becomes very difficult in certain cases. Take the example of variable arrays. The type of those arrays depend on the artificial DILocalVariable(s) which hold the size(s) of the array. This new function will now require that we generate the new variable and and new types. Similar issue exist for character type variables too. To avoid this after the fact updating, this PR generates a DISubprogramAttr for the TargetOp while generating the debug info in flang. This help us avoid updating later. This commit is flang side of the change.
In OMPIRBuilder, a new function is created for the TargetOp. We also create a new DISubprogram for it. All the variables that were in the target region now have to be updated to have the correct scope. This after the fact updating of debug information becomes very difficult in certain cases. This PR is making the change that OMPIRBuilder will not generate a DISubprogram. The responsibility has been shifted to the frontend. This allows us to simplify the updating of debug records. The commit is made a bit more complicated by the the fact that in new scheme, the debug location already points to the new DISubprogram by the time it reaches convertOmpTarget. But we need some code generation in the parent function so we have to carefully manage the debug locations. This fixes issue `llvm#134991`.
llvmbot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-mlir-openmp @llvm/pr-subscribers-mlir Author: Abid Qadeer (abidh) ChangesThis is combination of #138149 and #138039 which were opened separately for ease of reviewing. Only other change is adjustments in 2 tests which have gone in since. There are In To avoid this after the fact updating, this PR generates a This fixes issue Patch is 35.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146532.diff 14 Files Affected:
diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index 8fa2f38818c02..6eb914e67fd54 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -35,6 +35,7 @@
#include "llvm/BinaryFormat/Dwarf.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
@@ -104,6 +105,37 @@ bool debugInfoIsAlreadySet(mlir::Location loc) {
return false;
}
+// Generates the name for the artificial DISubprogram that we are going to
+// generate for omp::TargetOp. Its logic is borrowed from
+// getTargetEntryUniqueInfo and
+// TargetRegionEntryInfo::getTargetRegionEntryFnName to generate the same name.
+// But even if there was a slight mismatch, it is not a problem because this
+// name is artificial and not important to debug experience.
+mlir::StringAttr getTargetFunctionName(mlir::MLIRContext *context,
+ mlir::Location Loc,
+ llvm::StringRef parentName) {
+ auto fileLoc = Loc->findInstanceOf<mlir::FileLineColLoc>();
+
+ assert(fileLoc && "No file found from location");
+ llvm::StringRef fileName = fileLoc.getFilename().getValue();
+
+ llvm::sys::fs::UniqueID id;
+ uint64_t line = fileLoc.getLine();
+ size_t fileId;
+ size_t deviceId;
+ if (auto ec = llvm::sys::fs::getUniqueID(fileName, id)) {
+ fileId = llvm::hash_value(fileName.str());
+ deviceId = 0xdeadf17e;
+ } else {
+ fileId = id.getFile();
+ deviceId = id.getDevice();
+ }
+ return mlir::StringAttr::get(
+ context,
+ std::string(llvm::formatv("__omp_offloading_{0:x-}_{1:x-}_{2}_l{3}",
+ deviceId, fileId, parentName, line)));
+}
+
} // namespace
bool AddDebugInfoPass::createCommonBlockGlobal(
@@ -446,6 +478,79 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
line - 1, false);
}
+ auto addTargetOpDISP = [&](bool lineTableOnly,
+ llvm::ArrayRef<mlir::LLVM::DINodeAttr> entities) {
+ // When we process the DeclareOp inside the OpenMP target region, all the
+ // variables get the DISubprogram of the parent function of the target op as
+ // the scope. In the codegen (to llvm ir), OpenMP target op results in the
+ // creation of a separate function. As the variables in the debug info have
+ // the DISubprogram of the parent function as the scope, the variables
+ // need to be updated at codegen time to avoid verification failures.
+
+ // This updating after the fact becomes more and more difficult when types
+ // are dependent on local variables like in the case of variable size arrays
+ // or string. We not only have to generate new variables but also new types.
+ // We can avoid this problem by generating a DISubprogramAttr here for the
+ // target op and make sure that all the variables inside the target region
+ // get the correct scope in the first place.
+ funcOp.walk([&](mlir::omp::TargetOp targetOp) {
+ unsigned line = getLineFromLoc(targetOp.getLoc());
+ mlir::StringAttr name =
+ getTargetFunctionName(context, targetOp.getLoc(), funcOp.getName());
+ mlir::LLVM::DISubprogramFlags flags =
+ mlir::LLVM::DISubprogramFlags::Definition |
+ mlir::LLVM::DISubprogramFlags::LocalToUnit;
+ if (isOptimized)
+ flags = flags | mlir::LLVM::DISubprogramFlags::Optimized;
+
+ mlir::DistinctAttr id =
+ mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ llvm::SmallVector<mlir::LLVM::DITypeAttr> types;
+ types.push_back(mlir::LLVM::DINullTypeAttr::get(context));
+ for (auto arg : targetOp.getRegion().getArguments()) {
+ auto tyAttr = typeGen.convertType(fir::unwrapRefType(arg.getType()),
+ fileAttr, cuAttr, /*declOp=*/nullptr);
+ types.push_back(tyAttr);
+ }
+ CC = llvm::dwarf::getCallingConvention("DW_CC_normal");
+ mlir::LLVM::DISubroutineTypeAttr spTy =
+ mlir::LLVM::DISubroutineTypeAttr::get(context, CC, types);
+ if (lineTableOnly) {
+ auto spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, id, compilationUnit, Scope, name, name, funcFileAttr, line,
+ line, flags, spTy, /*retainedNodes=*/{}, /*annotations=*/{});
+ targetOp->setLoc(builder.getFusedLoc({targetOp.getLoc()}, spAttr));
+ return;
+ }
+ mlir::DistinctAttr recId =
+ mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ auto spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, recId, /*isRecSelf=*/true, id, compilationUnit, Scope, name,
+ name, funcFileAttr, line, line, flags, spTy, /*retainedNodes=*/{},
+ /*annotations=*/{});
+
+ // Make sure that information about the imported modules is copied in the
+ // new function.
+ llvm::SmallVector<mlir::LLVM::DINodeAttr> opEntities;
+ for (mlir::LLVM::DINodeAttr N : entities) {
+ if (auto entity = mlir::dyn_cast<mlir::LLVM::DIImportedEntityAttr>(N)) {
+ auto importedEntity = mlir::LLVM::DIImportedEntityAttr::get(
+ context, llvm::dwarf::DW_TAG_imported_module, spAttr,
+ entity.getEntity(), fileAttr, /*line=*/1, /*name=*/nullptr,
+ /*elements*/ {});
+ opEntities.push_back(importedEntity);
+ }
+ }
+
+ id = mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, recId, /*isRecSelf=*/false, id, compilationUnit, Scope, name,
+ name, funcFileAttr, line, line, flags, spTy, opEntities,
+ /*annotations=*/{});
+ targetOp->setLoc(builder.getFusedLoc({targetOp.getLoc()}, spAttr));
+ });
+ };
+
// Don't process variables if user asked for line tables only.
if (debugLevel == mlir::LLVM::DIEmissionKind::LineTablesOnly) {
auto spAttr = mlir::LLVM::DISubprogramAttr::get(
@@ -453,6 +558,7 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
line, line, subprogramFlags, subTypeAttr, /*retainedNodes=*/{},
/*annotations=*/{});
funcOp->setLoc(builder.getFusedLoc({l}, spAttr));
+ addTargetOpDISP(/*lineTableOnly=*/true, /*entities=*/{});
return;
}
@@ -510,9 +616,18 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
funcName, fullName, funcFileAttr, line, line, subprogramFlags,
subTypeAttr, entities, /*annotations=*/{});
funcOp->setLoc(builder.getFusedLoc({l}, spAttr));
+ addTargetOpDISP(/*lineTableOnly=*/false, entities);
funcOp.walk([&](fir::cg::XDeclareOp declOp) {
- handleDeclareOp(declOp, fileAttr, spAttr, typeGen, symbolTable);
+ mlir::LLVM::DISubprogramAttr spTy = spAttr;
+ if (auto tOp = declOp->getParentOfType<mlir::omp::TargetOp>()) {
+ if (auto fusedLoc = llvm::dyn_cast<mlir::FusedLoc>(tOp.getLoc())) {
+ if (auto sp = llvm::dyn_cast<mlir::LLVM::DISubprogramAttr>(
+ fusedLoc.getMetadata()))
+ spTy = sp;
+ }
+ }
+ handleDeclareOp(declOp, fileAttr, spTy, typeGen, symbolTable);
});
// commonBlockMap ensures that we don't create multiple DICommonBlockAttr of
// the same name in one function. But it is ok (rather required) to create
diff --git a/flang/test/Transforms/debug-omp-target-op-1.fir b/flang/test/Transforms/debug-omp-target-op-1.fir
new file mode 100644
index 0000000000000..6b895b732c42b
--- /dev/null
+++ b/flang/test/Transforms/debug-omp-target-op-1.fir
@@ -0,0 +1,40 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+// RUN: fir-opt --add-debug-info="debug-level=LineTablesOnly" --mlir-print-debuginfo %s | FileCheck %s --check-prefix=LINETABLE
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+ func.func @_QQmain() attributes {fir.bindc_name = "test"} {
+ %c13_i32 = arith.constant 13 : i32
+ %c12_i32 = arith.constant 12 : i32
+ %c6_i32 = arith.constant 6 : i32
+ %c1_i32 = arith.constant 1 : i32
+ %c5_i32 = arith.constant 5 : i32
+ %0 = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFEx"} loc(#loc1)
+ %1 = fircg.ext_declare %0 {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc1)
+ %2 = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFEy"} loc(#loc2)
+ %3 = fircg.ext_declare %2 {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc2)
+ %4 = omp.map.info var_ptr(%1 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "x"}
+ %5 = omp.map.info var_ptr(%3 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "y"}
+ omp.target map_entries(%4 -> %arg0, %5 -> %arg1 : !fir.ref<i32>, !fir.ref<i32>) {
+ %16 = fircg.ext_declare %arg0 {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc3)
+ %17 = fircg.ext_declare %arg1 {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc4)
+ omp.terminator
+ } loc(#loc5)
+ return
+ }
+}
+#loc1 = loc("test.f90":1:1)
+#loc2 = loc("test.f90":3:1)
+#loc3 = loc("test.f90":7:1)
+#loc4 = loc("test.f90":8:1)
+#loc5 = loc("test.f90":6:1)
+
+// CHECK: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "test"{{.*}}>
+// CHECK: #[[SP1:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_QQmain_l6"{{.*}}line = 6{{.*}}subprogramFlags = "LocalToUnit|Definition"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "x"{{.*}}line = 1, type = #[[TY:.*]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "y"{{.*}}line = 3, type = #[[TY]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP1]], name = "x"{{.*}}line = 7, type = #[[TY]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP1]], name = "y"{{.*}}line = 8, type = #[[TY]]>
+
+// LINETABLE: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "test"{{.*}}>
+// LINETABLE: #[[SP1:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_QQmain_l6"{{.*}}line = 6{{.*}}subprogramFlags = "LocalToUnit|Definition"{{.*}}>
+// LINETABLE-NOT: #llvm.di_local_variable
diff --git a/flang/test/Transforms/debug-omp-target-op-2.fir b/flang/test/Transforms/debug-omp-target-op-2.fir
new file mode 100644
index 0000000000000..15dcf2389b21d
--- /dev/null
+++ b/flang/test/Transforms/debug-omp-target-op-2.fir
@@ -0,0 +1,53 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+ func.func @fn_(%arg0: !fir.ref<!fir.array<?x?xi32>> {fir.bindc_name = "b"}, %arg1: !fir.ref<i32> {fir.bindc_name = "c"}, %arg2: !fir.ref<i32> {fir.bindc_name = "d"}) {
+ %c1 = arith.constant 1 : index
+ %c0 = arith.constant 0 : index
+ %0 = fir.alloca i32
+ %1 = fir.alloca i32
+ %2 = fir.undefined !fir.dscope
+ %3 = fircg.ext_declare %arg1 dummy_scope %2 {uniq_name = "_QFfnEc"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32> loc(#loc2)
+ %4 = fircg.ext_declare %arg2 dummy_scope %2 {uniq_name = "_QFfnEd"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32> loc(#loc3)
+ %5 = fir.load %3 : !fir.ref<i32>
+ %6 = fir.convert %5 : (i32) -> index
+ %9 = fir.load %4 : !fir.ref<i32>
+ %10 = fir.convert %9 : (i32) -> index
+ %15 = fircg.ext_declare %arg0(%6, %10) dummy_scope %2 {uniq_name = "_QFfnEb"} : (!fir.ref<!fir.array<?x?xi32>>, index, index, !fir.dscope) -> !fir.ref<!fir.array<?x?xi32>> loc(#loc4)
+ %16 = fircg.ext_embox %15(%6, %10) : (!fir.ref<!fir.array<?x?xi32>>, index, index) -> !fir.box<!fir.array<?x?xi32>>
+ %17:3 = fir.box_dims %16, %c0 : (!fir.box<!fir.array<?x?xi32>>, index) -> (index, index, index)
+ %18 = arith.subi %17#1, %c1 : index
+ %19 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%18 : index) extent(%17#1 : index) stride(%17#2 : index) start_idx(%c1 : index) {stride_in_bytes = true}
+ %20 = arith.muli %17#2, %17#1 : index
+ %21:3 = fir.box_dims %16, %c1 : (!fir.box<!fir.array<?x?xi32>>, index) -> (index, index, index)
+ %22 = arith.subi %21#1, %c1 : index
+ %23 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%22 : index) extent(%21#1 : index) stride(%20 : index) start_idx(%c1 : index) {stride_in_bytes = true}
+ %24 = omp.map.info var_ptr(%15 : !fir.ref<!fir.array<?x?xi32>>, i32) map_clauses(tofrom) capture(ByRef) bounds(%19, %23) -> !fir.ref<!fir.array<?x?xi32>> {name = "b"}
+ %25 = omp.map.info var_ptr(%1 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = ""}
+ %26 = omp.map.info var_ptr(%0 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = ""}
+ omp.target map_entries(%24 -> %arg3, %25 -> %arg4, %26 -> %arg5 : !fir.ref<!fir.array<?x?xi32>>, !fir.ref<i32>, !fir.ref<i32>) {
+ %27 = fir.load %arg5 : !fir.ref<i32>
+ %28 = fir.load %arg4 : !fir.ref<i32>
+ %29 = fir.convert %27 : (i32) -> index
+ %31 = fir.convert %28 : (i32) -> index
+ %37 = fircg.ext_declare %arg3(%29, %31) {uniq_name = "_QFfnEb"} : (!fir.ref<!fir.array<?x?xi32>>, index, index) -> !fir.ref<!fir.array<?x?xi32>> loc(#loc5)
+ omp.terminator
+ } loc(#loc6)
+ return
+ } loc(#loc7)
+}
+#loc1 = loc("test.f90":1:1)
+#loc2 = loc("test.f90":3:1)
+#loc3 = loc("test.f90":7:1)
+#loc4 = loc("test.f90":8:1)
+#loc5 = loc("test.f90":6:1)
+#loc6 = loc("test.f90":16:1)
+#loc7 = loc("test.f90":26:1)
+
+
+// Test that variable size arrays inside target regions get their own
+// compiler generated variables for size.
+
+// CHECK: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_fn__l16"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "._QFfnEb1"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "._QFfnEb2"{{.*}}>
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 85451b1233f96..db792a3b52d24 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -6891,23 +6891,19 @@ static void FixupDebugInfoForOutlinedFunction(
if (!NewSP)
return;
- DenseMap<const MDNode *, MDNode *> Cache;
SmallDenseMap<DILocalVariable *, DILocalVariable *> RemappedVariables;
auto GetUpdatedDIVariable = [&](DILocalVariable *OldVar, unsigned arg) {
- auto NewSP = Func->getSubprogram();
DILocalVariable *&NewVar = RemappedVariables[OldVar];
// 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());
+ Builder.getContext(), OldVar->getScope(), OldVar->getName(),
+ OldVar->getFile(), OldVar->getLine(), OldVar->getType(), arg,
+ OldVar->getFlags(), OldVar->getAlignInBits(), OldVar->getAnnotations());
return NewVar;
};
@@ -6921,7 +6917,8 @@ static void FixupDebugInfoForOutlinedFunction(
ArgNo = std::get<1>(Iter->second) + 1;
}
}
- DR->setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
+ if (ArgNo != 0)
+ DR->setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
};
// The location and scope of variable intrinsics and records still point to
@@ -7000,36 +6997,9 @@ static Expected<Function *> createOutlinedFunction(
// Save insert point.
IRBuilder<>::InsertPointGuard IPG(Builder);
- // If there's a DISubprogram associated with current function, then
- // generate one for the outlined function.
- if (Function *ParentFunc = BB->getParent()) {
- if (DISubprogram *SP = ParentFunc->getSubprogram()) {
- DICompileUnit *CU = SP->getUnit();
- DIBuilder DB(*M, true, CU);
- DebugLoc DL = Builder.getCurrentDebugLocation();
- if (DL) {
- // TODO: We are using nullopt for arguments at the moment. This will
- // need to be updated when debug data is being generated for variables.
- DISubroutineType *Ty =
- DB.createSubroutineType(DB.getOrCreateTypeArray({}));
- DISubprogram::DISPFlags SPFlags = DISubprogram::SPFlagDefinition |
- DISubprogram::SPFlagOptimized |
- DISubprogram::SPFlagLocalToUnit;
-
- DISubprogram *OutlinedSP = DB.createFunction(
- CU, FuncName, FuncName, SP->getFile(), DL.getLine(), Ty,
- DL.getLine(), DINode::DIFlags::FlagArtificial, SPFlags);
-
- // Attach subprogram to the function.
- Func->setSubprogram(OutlinedSP);
- // Update the CurrentDebugLocation in the builder so that right scope
- // is used for things inside outlined function.
- Builder.SetCurrentDebugLocation(
- DILocation::get(Func->getContext(), DL.getLine(), DL.getCol(),
- OutlinedSP, DL.getInlinedAt()));
- }
- }
- }
+ // We will generate the entries in the outlined function but the debug
+ // location may still be pointing to the parent function. Reset it now.
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
// Generate the region into the function.
BasicBlock *EntryBB = BasicBlock::Create(Builder.getContext(), "entry", Func);
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 3806db3ceab25..c1e1fec3ddef8 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -5324,9 +5324,27 @@ static LogicalResult
convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation) {
auto targetOp = cast<omp::TargetOp>(opInst);
+ // The current debug location already has the DISubprogram for the outlined
+ // function that will be created for the target op. We save it here so that
+ // we can set it on the outlined function.
+ llvm::DebugLoc outlinedFnLoc = builder.getCurrentDebugLocation();
if (failed(checkImplementationStatus(opInst)))
return failure();
+ // During the handling of target op, we will generate instructions in the
+ // parent function like call to the oulined function or branch to a new
+ // BasicBlock. We set the debug location here to parent function so that those
+ // get the correct debug locations. For outlined functions, the normal MLIR op
+ // conversion will automatically pick the correct location.
+ llvm::BasicBlock *parentBB = builder.GetInsertBlock();
+ assert(parentBB && "No insert block is set for the builder");
+ llvm::Function *parentLLVMFn = parentBB->getParent();
+ assert(parentLLVMFn && "Parent Function must be valid");
+ if (llvm::DISubprogram *SP = parentLLVMFn->getSubprogram())
+ builder.SetCurrentDebugLocation(llvm::DILocation::get(
+ parentLLVMFn->getContext(), outlinedFnLoc.getLine(),
+ outlinedFnLoc.getCol(), SP, outlinedFnLoc.getInlinedAt()));
+
llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
bool isTargetDevice = ompBuilder->Config.isTargetDevice();
bool isGPU = ompBuilder->Config.isGPU();
@@ -5420,6 +5438,9 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
assert(llvmParentFn && llvmOutlinedFn &&
"Both parent and outlined functions must exist at this point");
+ if (outlinedFnLoc && llvmParentFn->getSubprogram())
+ llvmOutlinedFn->setSubprogram(outlinedFnLoc->getScope()->getSubprogram());
+
if (auto attr = llvmParentFn->getFnAttribute("target-cpu");
attr.isStringAttribute())
llvmOutlinedFn->addFnAttr(attr);
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-empty.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-empty.mlir
new file mode 100644
index 0000000000000..45e5d2612e2c2
---...
[truncated]
|
|
@llvm/pr-subscribers-flang-openmp Author: Abid Qadeer (abidh) ChangesThis is combination of #138149 and #138039 which were opened separately for ease of reviewing. Only other change is adjustments in 2 tests which have gone in since. There are In To avoid this after the fact updating, this PR generates a This fixes issue Patch is 35.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146532.diff 14 Files Affected:
diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index 8fa2f38818c02..6eb914e67fd54 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -35,6 +35,7 @@
#include "llvm/BinaryFormat/Dwarf.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
@@ -104,6 +105,37 @@ bool debugInfoIsAlreadySet(mlir::Location loc) {
return false;
}
+// Generates the name for the artificial DISubprogram that we are going to
+// generate for omp::TargetOp. Its logic is borrowed from
+// getTargetEntryUniqueInfo and
+// TargetRegionEntryInfo::getTargetRegionEntryFnName to generate the same name.
+// But even if there was a slight mismatch, it is not a problem because this
+// name is artificial and not important to debug experience.
+mlir::StringAttr getTargetFunctionName(mlir::MLIRContext *context,
+ mlir::Location Loc,
+ llvm::StringRef parentName) {
+ auto fileLoc = Loc->findInstanceOf<mlir::FileLineColLoc>();
+
+ assert(fileLoc && "No file found from location");
+ llvm::StringRef fileName = fileLoc.getFilename().getValue();
+
+ llvm::sys::fs::UniqueID id;
+ uint64_t line = fileLoc.getLine();
+ size_t fileId;
+ size_t deviceId;
+ if (auto ec = llvm::sys::fs::getUniqueID(fileName, id)) {
+ fileId = llvm::hash_value(fileName.str());
+ deviceId = 0xdeadf17e;
+ } else {
+ fileId = id.getFile();
+ deviceId = id.getDevice();
+ }
+ return mlir::StringAttr::get(
+ context,
+ std::string(llvm::formatv("__omp_offloading_{0:x-}_{1:x-}_{2}_l{3}",
+ deviceId, fileId, parentName, line)));
+}
+
} // namespace
bool AddDebugInfoPass::createCommonBlockGlobal(
@@ -446,6 +478,79 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
line - 1, false);
}
+ auto addTargetOpDISP = [&](bool lineTableOnly,
+ llvm::ArrayRef<mlir::LLVM::DINodeAttr> entities) {
+ // When we process the DeclareOp inside the OpenMP target region, all the
+ // variables get the DISubprogram of the parent function of the target op as
+ // the scope. In the codegen (to llvm ir), OpenMP target op results in the
+ // creation of a separate function. As the variables in the debug info have
+ // the DISubprogram of the parent function as the scope, the variables
+ // need to be updated at codegen time to avoid verification failures.
+
+ // This updating after the fact becomes more and more difficult when types
+ // are dependent on local variables like in the case of variable size arrays
+ // or string. We not only have to generate new variables but also new types.
+ // We can avoid this problem by generating a DISubprogramAttr here for the
+ // target op and make sure that all the variables inside the target region
+ // get the correct scope in the first place.
+ funcOp.walk([&](mlir::omp::TargetOp targetOp) {
+ unsigned line = getLineFromLoc(targetOp.getLoc());
+ mlir::StringAttr name =
+ getTargetFunctionName(context, targetOp.getLoc(), funcOp.getName());
+ mlir::LLVM::DISubprogramFlags flags =
+ mlir::LLVM::DISubprogramFlags::Definition |
+ mlir::LLVM::DISubprogramFlags::LocalToUnit;
+ if (isOptimized)
+ flags = flags | mlir::LLVM::DISubprogramFlags::Optimized;
+
+ mlir::DistinctAttr id =
+ mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ llvm::SmallVector<mlir::LLVM::DITypeAttr> types;
+ types.push_back(mlir::LLVM::DINullTypeAttr::get(context));
+ for (auto arg : targetOp.getRegion().getArguments()) {
+ auto tyAttr = typeGen.convertType(fir::unwrapRefType(arg.getType()),
+ fileAttr, cuAttr, /*declOp=*/nullptr);
+ types.push_back(tyAttr);
+ }
+ CC = llvm::dwarf::getCallingConvention("DW_CC_normal");
+ mlir::LLVM::DISubroutineTypeAttr spTy =
+ mlir::LLVM::DISubroutineTypeAttr::get(context, CC, types);
+ if (lineTableOnly) {
+ auto spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, id, compilationUnit, Scope, name, name, funcFileAttr, line,
+ line, flags, spTy, /*retainedNodes=*/{}, /*annotations=*/{});
+ targetOp->setLoc(builder.getFusedLoc({targetOp.getLoc()}, spAttr));
+ return;
+ }
+ mlir::DistinctAttr recId =
+ mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ auto spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, recId, /*isRecSelf=*/true, id, compilationUnit, Scope, name,
+ name, funcFileAttr, line, line, flags, spTy, /*retainedNodes=*/{},
+ /*annotations=*/{});
+
+ // Make sure that information about the imported modules is copied in the
+ // new function.
+ llvm::SmallVector<mlir::LLVM::DINodeAttr> opEntities;
+ for (mlir::LLVM::DINodeAttr N : entities) {
+ if (auto entity = mlir::dyn_cast<mlir::LLVM::DIImportedEntityAttr>(N)) {
+ auto importedEntity = mlir::LLVM::DIImportedEntityAttr::get(
+ context, llvm::dwarf::DW_TAG_imported_module, spAttr,
+ entity.getEntity(), fileAttr, /*line=*/1, /*name=*/nullptr,
+ /*elements*/ {});
+ opEntities.push_back(importedEntity);
+ }
+ }
+
+ id = mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, recId, /*isRecSelf=*/false, id, compilationUnit, Scope, name,
+ name, funcFileAttr, line, line, flags, spTy, opEntities,
+ /*annotations=*/{});
+ targetOp->setLoc(builder.getFusedLoc({targetOp.getLoc()}, spAttr));
+ });
+ };
+
// Don't process variables if user asked for line tables only.
if (debugLevel == mlir::LLVM::DIEmissionKind::LineTablesOnly) {
auto spAttr = mlir::LLVM::DISubprogramAttr::get(
@@ -453,6 +558,7 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
line, line, subprogramFlags, subTypeAttr, /*retainedNodes=*/{},
/*annotations=*/{});
funcOp->setLoc(builder.getFusedLoc({l}, spAttr));
+ addTargetOpDISP(/*lineTableOnly=*/true, /*entities=*/{});
return;
}
@@ -510,9 +616,18 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
funcName, fullName, funcFileAttr, line, line, subprogramFlags,
subTypeAttr, entities, /*annotations=*/{});
funcOp->setLoc(builder.getFusedLoc({l}, spAttr));
+ addTargetOpDISP(/*lineTableOnly=*/false, entities);
funcOp.walk([&](fir::cg::XDeclareOp declOp) {
- handleDeclareOp(declOp, fileAttr, spAttr, typeGen, symbolTable);
+ mlir::LLVM::DISubprogramAttr spTy = spAttr;
+ if (auto tOp = declOp->getParentOfType<mlir::omp::TargetOp>()) {
+ if (auto fusedLoc = llvm::dyn_cast<mlir::FusedLoc>(tOp.getLoc())) {
+ if (auto sp = llvm::dyn_cast<mlir::LLVM::DISubprogramAttr>(
+ fusedLoc.getMetadata()))
+ spTy = sp;
+ }
+ }
+ handleDeclareOp(declOp, fileAttr, spTy, typeGen, symbolTable);
});
// commonBlockMap ensures that we don't create multiple DICommonBlockAttr of
// the same name in one function. But it is ok (rather required) to create
diff --git a/flang/test/Transforms/debug-omp-target-op-1.fir b/flang/test/Transforms/debug-omp-target-op-1.fir
new file mode 100644
index 0000000000000..6b895b732c42b
--- /dev/null
+++ b/flang/test/Transforms/debug-omp-target-op-1.fir
@@ -0,0 +1,40 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+// RUN: fir-opt --add-debug-info="debug-level=LineTablesOnly" --mlir-print-debuginfo %s | FileCheck %s --check-prefix=LINETABLE
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+ func.func @_QQmain() attributes {fir.bindc_name = "test"} {
+ %c13_i32 = arith.constant 13 : i32
+ %c12_i32 = arith.constant 12 : i32
+ %c6_i32 = arith.constant 6 : i32
+ %c1_i32 = arith.constant 1 : i32
+ %c5_i32 = arith.constant 5 : i32
+ %0 = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFEx"} loc(#loc1)
+ %1 = fircg.ext_declare %0 {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc1)
+ %2 = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFEy"} loc(#loc2)
+ %3 = fircg.ext_declare %2 {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc2)
+ %4 = omp.map.info var_ptr(%1 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "x"}
+ %5 = omp.map.info var_ptr(%3 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "y"}
+ omp.target map_entries(%4 -> %arg0, %5 -> %arg1 : !fir.ref<i32>, !fir.ref<i32>) {
+ %16 = fircg.ext_declare %arg0 {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc3)
+ %17 = fircg.ext_declare %arg1 {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc4)
+ omp.terminator
+ } loc(#loc5)
+ return
+ }
+}
+#loc1 = loc("test.f90":1:1)
+#loc2 = loc("test.f90":3:1)
+#loc3 = loc("test.f90":7:1)
+#loc4 = loc("test.f90":8:1)
+#loc5 = loc("test.f90":6:1)
+
+// CHECK: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "test"{{.*}}>
+// CHECK: #[[SP1:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_QQmain_l6"{{.*}}line = 6{{.*}}subprogramFlags = "LocalToUnit|Definition"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "x"{{.*}}line = 1, type = #[[TY:.*]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "y"{{.*}}line = 3, type = #[[TY]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP1]], name = "x"{{.*}}line = 7, type = #[[TY]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP1]], name = "y"{{.*}}line = 8, type = #[[TY]]>
+
+// LINETABLE: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "test"{{.*}}>
+// LINETABLE: #[[SP1:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_QQmain_l6"{{.*}}line = 6{{.*}}subprogramFlags = "LocalToUnit|Definition"{{.*}}>
+// LINETABLE-NOT: #llvm.di_local_variable
diff --git a/flang/test/Transforms/debug-omp-target-op-2.fir b/flang/test/Transforms/debug-omp-target-op-2.fir
new file mode 100644
index 0000000000000..15dcf2389b21d
--- /dev/null
+++ b/flang/test/Transforms/debug-omp-target-op-2.fir
@@ -0,0 +1,53 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+ func.func @fn_(%arg0: !fir.ref<!fir.array<?x?xi32>> {fir.bindc_name = "b"}, %arg1: !fir.ref<i32> {fir.bindc_name = "c"}, %arg2: !fir.ref<i32> {fir.bindc_name = "d"}) {
+ %c1 = arith.constant 1 : index
+ %c0 = arith.constant 0 : index
+ %0 = fir.alloca i32
+ %1 = fir.alloca i32
+ %2 = fir.undefined !fir.dscope
+ %3 = fircg.ext_declare %arg1 dummy_scope %2 {uniq_name = "_QFfnEc"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32> loc(#loc2)
+ %4 = fircg.ext_declare %arg2 dummy_scope %2 {uniq_name = "_QFfnEd"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32> loc(#loc3)
+ %5 = fir.load %3 : !fir.ref<i32>
+ %6 = fir.convert %5 : (i32) -> index
+ %9 = fir.load %4 : !fir.ref<i32>
+ %10 = fir.convert %9 : (i32) -> index
+ %15 = fircg.ext_declare %arg0(%6, %10) dummy_scope %2 {uniq_name = "_QFfnEb"} : (!fir.ref<!fir.array<?x?xi32>>, index, index, !fir.dscope) -> !fir.ref<!fir.array<?x?xi32>> loc(#loc4)
+ %16 = fircg.ext_embox %15(%6, %10) : (!fir.ref<!fir.array<?x?xi32>>, index, index) -> !fir.box<!fir.array<?x?xi32>>
+ %17:3 = fir.box_dims %16, %c0 : (!fir.box<!fir.array<?x?xi32>>, index) -> (index, index, index)
+ %18 = arith.subi %17#1, %c1 : index
+ %19 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%18 : index) extent(%17#1 : index) stride(%17#2 : index) start_idx(%c1 : index) {stride_in_bytes = true}
+ %20 = arith.muli %17#2, %17#1 : index
+ %21:3 = fir.box_dims %16, %c1 : (!fir.box<!fir.array<?x?xi32>>, index) -> (index, index, index)
+ %22 = arith.subi %21#1, %c1 : index
+ %23 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%22 : index) extent(%21#1 : index) stride(%20 : index) start_idx(%c1 : index) {stride_in_bytes = true}
+ %24 = omp.map.info var_ptr(%15 : !fir.ref<!fir.array<?x?xi32>>, i32) map_clauses(tofrom) capture(ByRef) bounds(%19, %23) -> !fir.ref<!fir.array<?x?xi32>> {name = "b"}
+ %25 = omp.map.info var_ptr(%1 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = ""}
+ %26 = omp.map.info var_ptr(%0 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = ""}
+ omp.target map_entries(%24 -> %arg3, %25 -> %arg4, %26 -> %arg5 : !fir.ref<!fir.array<?x?xi32>>, !fir.ref<i32>, !fir.ref<i32>) {
+ %27 = fir.load %arg5 : !fir.ref<i32>
+ %28 = fir.load %arg4 : !fir.ref<i32>
+ %29 = fir.convert %27 : (i32) -> index
+ %31 = fir.convert %28 : (i32) -> index
+ %37 = fircg.ext_declare %arg3(%29, %31) {uniq_name = "_QFfnEb"} : (!fir.ref<!fir.array<?x?xi32>>, index, index) -> !fir.ref<!fir.array<?x?xi32>> loc(#loc5)
+ omp.terminator
+ } loc(#loc6)
+ return
+ } loc(#loc7)
+}
+#loc1 = loc("test.f90":1:1)
+#loc2 = loc("test.f90":3:1)
+#loc3 = loc("test.f90":7:1)
+#loc4 = loc("test.f90":8:1)
+#loc5 = loc("test.f90":6:1)
+#loc6 = loc("test.f90":16:1)
+#loc7 = loc("test.f90":26:1)
+
+
+// Test that variable size arrays inside target regions get their own
+// compiler generated variables for size.
+
+// CHECK: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_fn__l16"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "._QFfnEb1"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "._QFfnEb2"{{.*}}>
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 85451b1233f96..db792a3b52d24 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -6891,23 +6891,19 @@ static void FixupDebugInfoForOutlinedFunction(
if (!NewSP)
return;
- DenseMap<const MDNode *, MDNode *> Cache;
SmallDenseMap<DILocalVariable *, DILocalVariable *> RemappedVariables;
auto GetUpdatedDIVariable = [&](DILocalVariable *OldVar, unsigned arg) {
- auto NewSP = Func->getSubprogram();
DILocalVariable *&NewVar = RemappedVariables[OldVar];
// 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());
+ Builder.getContext(), OldVar->getScope(), OldVar->getName(),
+ OldVar->getFile(), OldVar->getLine(), OldVar->getType(), arg,
+ OldVar->getFlags(), OldVar->getAlignInBits(), OldVar->getAnnotations());
return NewVar;
};
@@ -6921,7 +6917,8 @@ static void FixupDebugInfoForOutlinedFunction(
ArgNo = std::get<1>(Iter->second) + 1;
}
}
- DR->setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
+ if (ArgNo != 0)
+ DR->setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
};
// The location and scope of variable intrinsics and records still point to
@@ -7000,36 +6997,9 @@ static Expected<Function *> createOutlinedFunction(
// Save insert point.
IRBuilder<>::InsertPointGuard IPG(Builder);
- // If there's a DISubprogram associated with current function, then
- // generate one for the outlined function.
- if (Function *ParentFunc = BB->getParent()) {
- if (DISubprogram *SP = ParentFunc->getSubprogram()) {
- DICompileUnit *CU = SP->getUnit();
- DIBuilder DB(*M, true, CU);
- DebugLoc DL = Builder.getCurrentDebugLocation();
- if (DL) {
- // TODO: We are using nullopt for arguments at the moment. This will
- // need to be updated when debug data is being generated for variables.
- DISubroutineType *Ty =
- DB.createSubroutineType(DB.getOrCreateTypeArray({}));
- DISubprogram::DISPFlags SPFlags = DISubprogram::SPFlagDefinition |
- DISubprogram::SPFlagOptimized |
- DISubprogram::SPFlagLocalToUnit;
-
- DISubprogram *OutlinedSP = DB.createFunction(
- CU, FuncName, FuncName, SP->getFile(), DL.getLine(), Ty,
- DL.getLine(), DINode::DIFlags::FlagArtificial, SPFlags);
-
- // Attach subprogram to the function.
- Func->setSubprogram(OutlinedSP);
- // Update the CurrentDebugLocation in the builder so that right scope
- // is used for things inside outlined function.
- Builder.SetCurrentDebugLocation(
- DILocation::get(Func->getContext(), DL.getLine(), DL.getCol(),
- OutlinedSP, DL.getInlinedAt()));
- }
- }
- }
+ // We will generate the entries in the outlined function but the debug
+ // location may still be pointing to the parent function. Reset it now.
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
// Generate the region into the function.
BasicBlock *EntryBB = BasicBlock::Create(Builder.getContext(), "entry", Func);
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 3806db3ceab25..c1e1fec3ddef8 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -5324,9 +5324,27 @@ static LogicalResult
convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation) {
auto targetOp = cast<omp::TargetOp>(opInst);
+ // The current debug location already has the DISubprogram for the outlined
+ // function that will be created for the target op. We save it here so that
+ // we can set it on the outlined function.
+ llvm::DebugLoc outlinedFnLoc = builder.getCurrentDebugLocation();
if (failed(checkImplementationStatus(opInst)))
return failure();
+ // During the handling of target op, we will generate instructions in the
+ // parent function like call to the oulined function or branch to a new
+ // BasicBlock. We set the debug location here to parent function so that those
+ // get the correct debug locations. For outlined functions, the normal MLIR op
+ // conversion will automatically pick the correct location.
+ llvm::BasicBlock *parentBB = builder.GetInsertBlock();
+ assert(parentBB && "No insert block is set for the builder");
+ llvm::Function *parentLLVMFn = parentBB->getParent();
+ assert(parentLLVMFn && "Parent Function must be valid");
+ if (llvm::DISubprogram *SP = parentLLVMFn->getSubprogram())
+ builder.SetCurrentDebugLocation(llvm::DILocation::get(
+ parentLLVMFn->getContext(), outlinedFnLoc.getLine(),
+ outlinedFnLoc.getCol(), SP, outlinedFnLoc.getInlinedAt()));
+
llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
bool isTargetDevice = ompBuilder->Config.isTargetDevice();
bool isGPU = ompBuilder->Config.isGPU();
@@ -5420,6 +5438,9 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
assert(llvmParentFn && llvmOutlinedFn &&
"Both parent and outlined functions must exist at this point");
+ if (outlinedFnLoc && llvmParentFn->getSubprogram())
+ llvmOutlinedFn->setSubprogram(outlinedFnLoc->getScope()->getSubprogram());
+
if (auto attr = llvmParentFn->getFnAttribute("target-cpu");
attr.isStringAttribute())
llvmOutlinedFn->addFnAttr(attr);
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-empty.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-empty.mlir
new file mode 100644
index 0000000000000..45e5d2612e2c2
---...
[truncated]
|
|
@llvm/pr-subscribers-mlir-llvm Author: Abid Qadeer (abidh) ChangesThis is combination of #138149 and #138039 which were opened separately for ease of reviewing. Only other change is adjustments in 2 tests which have gone in since. There are In To avoid this after the fact updating, this PR generates a This fixes issue Patch is 35.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146532.diff 14 Files Affected:
diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index 8fa2f38818c02..6eb914e67fd54 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -35,6 +35,7 @@
#include "llvm/BinaryFormat/Dwarf.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
@@ -104,6 +105,37 @@ bool debugInfoIsAlreadySet(mlir::Location loc) {
return false;
}
+// Generates the name for the artificial DISubprogram that we are going to
+// generate for omp::TargetOp. Its logic is borrowed from
+// getTargetEntryUniqueInfo and
+// TargetRegionEntryInfo::getTargetRegionEntryFnName to generate the same name.
+// But even if there was a slight mismatch, it is not a problem because this
+// name is artificial and not important to debug experience.
+mlir::StringAttr getTargetFunctionName(mlir::MLIRContext *context,
+ mlir::Location Loc,
+ llvm::StringRef parentName) {
+ auto fileLoc = Loc->findInstanceOf<mlir::FileLineColLoc>();
+
+ assert(fileLoc && "No file found from location");
+ llvm::StringRef fileName = fileLoc.getFilename().getValue();
+
+ llvm::sys::fs::UniqueID id;
+ uint64_t line = fileLoc.getLine();
+ size_t fileId;
+ size_t deviceId;
+ if (auto ec = llvm::sys::fs::getUniqueID(fileName, id)) {
+ fileId = llvm::hash_value(fileName.str());
+ deviceId = 0xdeadf17e;
+ } else {
+ fileId = id.getFile();
+ deviceId = id.getDevice();
+ }
+ return mlir::StringAttr::get(
+ context,
+ std::string(llvm::formatv("__omp_offloading_{0:x-}_{1:x-}_{2}_l{3}",
+ deviceId, fileId, parentName, line)));
+}
+
} // namespace
bool AddDebugInfoPass::createCommonBlockGlobal(
@@ -446,6 +478,79 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
line - 1, false);
}
+ auto addTargetOpDISP = [&](bool lineTableOnly,
+ llvm::ArrayRef<mlir::LLVM::DINodeAttr> entities) {
+ // When we process the DeclareOp inside the OpenMP target region, all the
+ // variables get the DISubprogram of the parent function of the target op as
+ // the scope. In the codegen (to llvm ir), OpenMP target op results in the
+ // creation of a separate function. As the variables in the debug info have
+ // the DISubprogram of the parent function as the scope, the variables
+ // need to be updated at codegen time to avoid verification failures.
+
+ // This updating after the fact becomes more and more difficult when types
+ // are dependent on local variables like in the case of variable size arrays
+ // or string. We not only have to generate new variables but also new types.
+ // We can avoid this problem by generating a DISubprogramAttr here for the
+ // target op and make sure that all the variables inside the target region
+ // get the correct scope in the first place.
+ funcOp.walk([&](mlir::omp::TargetOp targetOp) {
+ unsigned line = getLineFromLoc(targetOp.getLoc());
+ mlir::StringAttr name =
+ getTargetFunctionName(context, targetOp.getLoc(), funcOp.getName());
+ mlir::LLVM::DISubprogramFlags flags =
+ mlir::LLVM::DISubprogramFlags::Definition |
+ mlir::LLVM::DISubprogramFlags::LocalToUnit;
+ if (isOptimized)
+ flags = flags | mlir::LLVM::DISubprogramFlags::Optimized;
+
+ mlir::DistinctAttr id =
+ mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ llvm::SmallVector<mlir::LLVM::DITypeAttr> types;
+ types.push_back(mlir::LLVM::DINullTypeAttr::get(context));
+ for (auto arg : targetOp.getRegion().getArguments()) {
+ auto tyAttr = typeGen.convertType(fir::unwrapRefType(arg.getType()),
+ fileAttr, cuAttr, /*declOp=*/nullptr);
+ types.push_back(tyAttr);
+ }
+ CC = llvm::dwarf::getCallingConvention("DW_CC_normal");
+ mlir::LLVM::DISubroutineTypeAttr spTy =
+ mlir::LLVM::DISubroutineTypeAttr::get(context, CC, types);
+ if (lineTableOnly) {
+ auto spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, id, compilationUnit, Scope, name, name, funcFileAttr, line,
+ line, flags, spTy, /*retainedNodes=*/{}, /*annotations=*/{});
+ targetOp->setLoc(builder.getFusedLoc({targetOp.getLoc()}, spAttr));
+ return;
+ }
+ mlir::DistinctAttr recId =
+ mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ auto spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, recId, /*isRecSelf=*/true, id, compilationUnit, Scope, name,
+ name, funcFileAttr, line, line, flags, spTy, /*retainedNodes=*/{},
+ /*annotations=*/{});
+
+ // Make sure that information about the imported modules is copied in the
+ // new function.
+ llvm::SmallVector<mlir::LLVM::DINodeAttr> opEntities;
+ for (mlir::LLVM::DINodeAttr N : entities) {
+ if (auto entity = mlir::dyn_cast<mlir::LLVM::DIImportedEntityAttr>(N)) {
+ auto importedEntity = mlir::LLVM::DIImportedEntityAttr::get(
+ context, llvm::dwarf::DW_TAG_imported_module, spAttr,
+ entity.getEntity(), fileAttr, /*line=*/1, /*name=*/nullptr,
+ /*elements*/ {});
+ opEntities.push_back(importedEntity);
+ }
+ }
+
+ id = mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+ spAttr = mlir::LLVM::DISubprogramAttr::get(
+ context, recId, /*isRecSelf=*/false, id, compilationUnit, Scope, name,
+ name, funcFileAttr, line, line, flags, spTy, opEntities,
+ /*annotations=*/{});
+ targetOp->setLoc(builder.getFusedLoc({targetOp.getLoc()}, spAttr));
+ });
+ };
+
// Don't process variables if user asked for line tables only.
if (debugLevel == mlir::LLVM::DIEmissionKind::LineTablesOnly) {
auto spAttr = mlir::LLVM::DISubprogramAttr::get(
@@ -453,6 +558,7 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
line, line, subprogramFlags, subTypeAttr, /*retainedNodes=*/{},
/*annotations=*/{});
funcOp->setLoc(builder.getFusedLoc({l}, spAttr));
+ addTargetOpDISP(/*lineTableOnly=*/true, /*entities=*/{});
return;
}
@@ -510,9 +616,18 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
funcName, fullName, funcFileAttr, line, line, subprogramFlags,
subTypeAttr, entities, /*annotations=*/{});
funcOp->setLoc(builder.getFusedLoc({l}, spAttr));
+ addTargetOpDISP(/*lineTableOnly=*/false, entities);
funcOp.walk([&](fir::cg::XDeclareOp declOp) {
- handleDeclareOp(declOp, fileAttr, spAttr, typeGen, symbolTable);
+ mlir::LLVM::DISubprogramAttr spTy = spAttr;
+ if (auto tOp = declOp->getParentOfType<mlir::omp::TargetOp>()) {
+ if (auto fusedLoc = llvm::dyn_cast<mlir::FusedLoc>(tOp.getLoc())) {
+ if (auto sp = llvm::dyn_cast<mlir::LLVM::DISubprogramAttr>(
+ fusedLoc.getMetadata()))
+ spTy = sp;
+ }
+ }
+ handleDeclareOp(declOp, fileAttr, spTy, typeGen, symbolTable);
});
// commonBlockMap ensures that we don't create multiple DICommonBlockAttr of
// the same name in one function. But it is ok (rather required) to create
diff --git a/flang/test/Transforms/debug-omp-target-op-1.fir b/flang/test/Transforms/debug-omp-target-op-1.fir
new file mode 100644
index 0000000000000..6b895b732c42b
--- /dev/null
+++ b/flang/test/Transforms/debug-omp-target-op-1.fir
@@ -0,0 +1,40 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+// RUN: fir-opt --add-debug-info="debug-level=LineTablesOnly" --mlir-print-debuginfo %s | FileCheck %s --check-prefix=LINETABLE
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+ func.func @_QQmain() attributes {fir.bindc_name = "test"} {
+ %c13_i32 = arith.constant 13 : i32
+ %c12_i32 = arith.constant 12 : i32
+ %c6_i32 = arith.constant 6 : i32
+ %c1_i32 = arith.constant 1 : i32
+ %c5_i32 = arith.constant 5 : i32
+ %0 = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFEx"} loc(#loc1)
+ %1 = fircg.ext_declare %0 {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc1)
+ %2 = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFEy"} loc(#loc2)
+ %3 = fircg.ext_declare %2 {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc2)
+ %4 = omp.map.info var_ptr(%1 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "x"}
+ %5 = omp.map.info var_ptr(%3 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "y"}
+ omp.target map_entries(%4 -> %arg0, %5 -> %arg1 : !fir.ref<i32>, !fir.ref<i32>) {
+ %16 = fircg.ext_declare %arg0 {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc3)
+ %17 = fircg.ext_declare %arg1 {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> !fir.ref<i32> loc(#loc4)
+ omp.terminator
+ } loc(#loc5)
+ return
+ }
+}
+#loc1 = loc("test.f90":1:1)
+#loc2 = loc("test.f90":3:1)
+#loc3 = loc("test.f90":7:1)
+#loc4 = loc("test.f90":8:1)
+#loc5 = loc("test.f90":6:1)
+
+// CHECK: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "test"{{.*}}>
+// CHECK: #[[SP1:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_QQmain_l6"{{.*}}line = 6{{.*}}subprogramFlags = "LocalToUnit|Definition"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "x"{{.*}}line = 1, type = #[[TY:.*]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "y"{{.*}}line = 3, type = #[[TY]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP1]], name = "x"{{.*}}line = 7, type = #[[TY]]>
+// CHECK: #llvm.di_local_variable<scope = #[[SP1]], name = "y"{{.*}}line = 8, type = #[[TY]]>
+
+// LINETABLE: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "test"{{.*}}>
+// LINETABLE: #[[SP1:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_QQmain_l6"{{.*}}line = 6{{.*}}subprogramFlags = "LocalToUnit|Definition"{{.*}}>
+// LINETABLE-NOT: #llvm.di_local_variable
diff --git a/flang/test/Transforms/debug-omp-target-op-2.fir b/flang/test/Transforms/debug-omp-target-op-2.fir
new file mode 100644
index 0000000000000..15dcf2389b21d
--- /dev/null
+++ b/flang/test/Transforms/debug-omp-target-op-2.fir
@@ -0,0 +1,53 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+ func.func @fn_(%arg0: !fir.ref<!fir.array<?x?xi32>> {fir.bindc_name = "b"}, %arg1: !fir.ref<i32> {fir.bindc_name = "c"}, %arg2: !fir.ref<i32> {fir.bindc_name = "d"}) {
+ %c1 = arith.constant 1 : index
+ %c0 = arith.constant 0 : index
+ %0 = fir.alloca i32
+ %1 = fir.alloca i32
+ %2 = fir.undefined !fir.dscope
+ %3 = fircg.ext_declare %arg1 dummy_scope %2 {uniq_name = "_QFfnEc"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32> loc(#loc2)
+ %4 = fircg.ext_declare %arg2 dummy_scope %2 {uniq_name = "_QFfnEd"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32> loc(#loc3)
+ %5 = fir.load %3 : !fir.ref<i32>
+ %6 = fir.convert %5 : (i32) -> index
+ %9 = fir.load %4 : !fir.ref<i32>
+ %10 = fir.convert %9 : (i32) -> index
+ %15 = fircg.ext_declare %arg0(%6, %10) dummy_scope %2 {uniq_name = "_QFfnEb"} : (!fir.ref<!fir.array<?x?xi32>>, index, index, !fir.dscope) -> !fir.ref<!fir.array<?x?xi32>> loc(#loc4)
+ %16 = fircg.ext_embox %15(%6, %10) : (!fir.ref<!fir.array<?x?xi32>>, index, index) -> !fir.box<!fir.array<?x?xi32>>
+ %17:3 = fir.box_dims %16, %c0 : (!fir.box<!fir.array<?x?xi32>>, index) -> (index, index, index)
+ %18 = arith.subi %17#1, %c1 : index
+ %19 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%18 : index) extent(%17#1 : index) stride(%17#2 : index) start_idx(%c1 : index) {stride_in_bytes = true}
+ %20 = arith.muli %17#2, %17#1 : index
+ %21:3 = fir.box_dims %16, %c1 : (!fir.box<!fir.array<?x?xi32>>, index) -> (index, index, index)
+ %22 = arith.subi %21#1, %c1 : index
+ %23 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%22 : index) extent(%21#1 : index) stride(%20 : index) start_idx(%c1 : index) {stride_in_bytes = true}
+ %24 = omp.map.info var_ptr(%15 : !fir.ref<!fir.array<?x?xi32>>, i32) map_clauses(tofrom) capture(ByRef) bounds(%19, %23) -> !fir.ref<!fir.array<?x?xi32>> {name = "b"}
+ %25 = omp.map.info var_ptr(%1 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = ""}
+ %26 = omp.map.info var_ptr(%0 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = ""}
+ omp.target map_entries(%24 -> %arg3, %25 -> %arg4, %26 -> %arg5 : !fir.ref<!fir.array<?x?xi32>>, !fir.ref<i32>, !fir.ref<i32>) {
+ %27 = fir.load %arg5 : !fir.ref<i32>
+ %28 = fir.load %arg4 : !fir.ref<i32>
+ %29 = fir.convert %27 : (i32) -> index
+ %31 = fir.convert %28 : (i32) -> index
+ %37 = fircg.ext_declare %arg3(%29, %31) {uniq_name = "_QFfnEb"} : (!fir.ref<!fir.array<?x?xi32>>, index, index) -> !fir.ref<!fir.array<?x?xi32>> loc(#loc5)
+ omp.terminator
+ } loc(#loc6)
+ return
+ } loc(#loc7)
+}
+#loc1 = loc("test.f90":1:1)
+#loc2 = loc("test.f90":3:1)
+#loc3 = loc("test.f90":7:1)
+#loc4 = loc("test.f90":8:1)
+#loc5 = loc("test.f90":6:1)
+#loc6 = loc("test.f90":16:1)
+#loc7 = loc("test.f90":26:1)
+
+
+// Test that variable size arrays inside target regions get their own
+// compiler generated variables for size.
+
+// CHECK: #[[SP:.*]] = #llvm.di_subprogram<{{.*}}name = "__omp_offloading_{{.*}}_fn__l16"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "._QFfnEb1"{{.*}}>
+// CHECK: #llvm.di_local_variable<scope = #[[SP]], name = "._QFfnEb2"{{.*}}>
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 85451b1233f96..db792a3b52d24 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -6891,23 +6891,19 @@ static void FixupDebugInfoForOutlinedFunction(
if (!NewSP)
return;
- DenseMap<const MDNode *, MDNode *> Cache;
SmallDenseMap<DILocalVariable *, DILocalVariable *> RemappedVariables;
auto GetUpdatedDIVariable = [&](DILocalVariable *OldVar, unsigned arg) {
- auto NewSP = Func->getSubprogram();
DILocalVariable *&NewVar = RemappedVariables[OldVar];
// 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());
+ Builder.getContext(), OldVar->getScope(), OldVar->getName(),
+ OldVar->getFile(), OldVar->getLine(), OldVar->getType(), arg,
+ OldVar->getFlags(), OldVar->getAlignInBits(), OldVar->getAnnotations());
return NewVar;
};
@@ -6921,7 +6917,8 @@ static void FixupDebugInfoForOutlinedFunction(
ArgNo = std::get<1>(Iter->second) + 1;
}
}
- DR->setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
+ if (ArgNo != 0)
+ DR->setVariable(GetUpdatedDIVariable(OldVar, ArgNo));
};
// The location and scope of variable intrinsics and records still point to
@@ -7000,36 +6997,9 @@ static Expected<Function *> createOutlinedFunction(
// Save insert point.
IRBuilder<>::InsertPointGuard IPG(Builder);
- // If there's a DISubprogram associated with current function, then
- // generate one for the outlined function.
- if (Function *ParentFunc = BB->getParent()) {
- if (DISubprogram *SP = ParentFunc->getSubprogram()) {
- DICompileUnit *CU = SP->getUnit();
- DIBuilder DB(*M, true, CU);
- DebugLoc DL = Builder.getCurrentDebugLocation();
- if (DL) {
- // TODO: We are using nullopt for arguments at the moment. This will
- // need to be updated when debug data is being generated for variables.
- DISubroutineType *Ty =
- DB.createSubroutineType(DB.getOrCreateTypeArray({}));
- DISubprogram::DISPFlags SPFlags = DISubprogram::SPFlagDefinition |
- DISubprogram::SPFlagOptimized |
- DISubprogram::SPFlagLocalToUnit;
-
- DISubprogram *OutlinedSP = DB.createFunction(
- CU, FuncName, FuncName, SP->getFile(), DL.getLine(), Ty,
- DL.getLine(), DINode::DIFlags::FlagArtificial, SPFlags);
-
- // Attach subprogram to the function.
- Func->setSubprogram(OutlinedSP);
- // Update the CurrentDebugLocation in the builder so that right scope
- // is used for things inside outlined function.
- Builder.SetCurrentDebugLocation(
- DILocation::get(Func->getContext(), DL.getLine(), DL.getCol(),
- OutlinedSP, DL.getInlinedAt()));
- }
- }
- }
+ // We will generate the entries in the outlined function but the debug
+ // location may still be pointing to the parent function. Reset it now.
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
// Generate the region into the function.
BasicBlock *EntryBB = BasicBlock::Create(Builder.getContext(), "entry", Func);
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 3806db3ceab25..c1e1fec3ddef8 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -5324,9 +5324,27 @@ static LogicalResult
convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
LLVM::ModuleTranslation &moduleTranslation) {
auto targetOp = cast<omp::TargetOp>(opInst);
+ // The current debug location already has the DISubprogram for the outlined
+ // function that will be created for the target op. We save it here so that
+ // we can set it on the outlined function.
+ llvm::DebugLoc outlinedFnLoc = builder.getCurrentDebugLocation();
if (failed(checkImplementationStatus(opInst)))
return failure();
+ // During the handling of target op, we will generate instructions in the
+ // parent function like call to the oulined function or branch to a new
+ // BasicBlock. We set the debug location here to parent function so that those
+ // get the correct debug locations. For outlined functions, the normal MLIR op
+ // conversion will automatically pick the correct location.
+ llvm::BasicBlock *parentBB = builder.GetInsertBlock();
+ assert(parentBB && "No insert block is set for the builder");
+ llvm::Function *parentLLVMFn = parentBB->getParent();
+ assert(parentLLVMFn && "Parent Function must be valid");
+ if (llvm::DISubprogram *SP = parentLLVMFn->getSubprogram())
+ builder.SetCurrentDebugLocation(llvm::DILocation::get(
+ parentLLVMFn->getContext(), outlinedFnLoc.getLine(),
+ outlinedFnLoc.getCol(), SP, outlinedFnLoc.getInlinedAt()));
+
llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
bool isTargetDevice = ompBuilder->Config.isTargetDevice();
bool isGPU = ompBuilder->Config.isGPU();
@@ -5420,6 +5438,9 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
assert(llvmParentFn && llvmOutlinedFn &&
"Both parent and outlined functions must exist at this point");
+ if (outlinedFnLoc && llvmParentFn->getSubprogram())
+ llvmOutlinedFn->setSubprogram(outlinedFnLoc->getScope()->getSubprogram());
+
if (auto attr = llvmParentFn->getFnAttribute("target-cpu");
attr.isStringAttribute())
llvmOutlinedFn->addFnAttr(attr);
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-empty.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-empty.mlir
new file mode 100644
index 0000000000000..45e5d2612e2c2
---...
[truncated]
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors debug-info handling for OpenMP TargetOp by generating a DISubprogramAttr early in Flang, updating the MLIR OpenMP-to-LLVMIR translation to carry that attr into the outlined function, and simplifying LLVM’s OMPIRBuilder debug-fixup logic. It also updates a number of MLIR and FIR tests to check for the new subprogram and location metadata.
- Generate
DISubprogramAttrfor eachomp::TargetOpinflang/lib/Optimizer/Transforms/AddDebugInfo.cpp. - In
OpenMPToLLVMIRTranslation.cpp, capture the original debug location, switch to the parent function while emitting host-side calls, and apply the correct subprogram to the outlined function. - Remove scope cloning in
OMPIRBuilder.cppand reset the debug location before generating the outlined region. - Update MLIR and FIR tests under
mlir/test/Target/LLVMIRandflang/test/Transformsto expect the new<#sp1>entries and fused locations.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | Save/restore debug loc and set outlined function subprogram |
| llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | Remove scope clone, reset debug location for outlined code |
| flang/lib/Optimizer/Transforms/AddDebugInfo.cpp | Add getTargetFunctionName & addTargetOpDISP for target ops |
| mlir/test/Target/LLVMIR/*.mlir | Update tests for new #sp1 and fused locations |
| flang/test/Transforms/debug-omp-target-op-*.fir | Add FIR tests to cover target-region debug for size-dependent vars |
Comments suppressed due to low confidence (1)
mlir/test/Target/LLVMIR/omptarget-parallel-llvm-debug.mlir:9
- The new
#sp1DISubprogram lacks anid = distinct[...]<>field, unlike the other subprogram entries. Adding a distinct ID will keep the test consistent and ensure IR verification.
#sp1 = #llvm.di_subprogram<compileUnit = #cu, scope = #di_file, name = "kernel", file = #di_file, subprogramFlags = "Definition", type = #sp_ty>
Comment on lines
6903
to
+6904
| NewVar = llvm::DILocalVariable::get( | ||
| Builder.getContext(), NewScope, OldVar->getName(), OldVar->getFile(), | ||
| OldVar->getLine(), OldVar->getType(), arg, OldVar->getFlags(), | ||
| OldVar->getAlignInBits(), OldVar->getAnnotations()); | ||
| Builder.getContext(), OldVar->getScope(), OldVar->getName(), |
There was a problem hiding this comment.
The new local-variable fixup is still using the old scope (OldVar->getScope()) instead of the outlined function's DISubprogram scope. You should clone or otherwise obtain a scope rooted at the new SP and pass that to DILocalVariable::get, as in the original cloneScopeForSubprogram approach.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Using OldVar->getScope() is the right thing here.
| llvm::Function *parentLLVMFn = parentBB->getParent(); | ||
| assert(parentLLVMFn && "Parent Function must be valid"); | ||
| if (llvm::DISubprogram *SP = parentLLVMFn->getSubprogram()) | ||
| builder.SetCurrentDebugLocation(llvm::DILocation::get( |
There was a problem hiding this comment.
After switching the debug location to the parent function for host-side calls, you should restore the original outlinedFnLoc once you finish emitting those calls to avoid polluting subsequent IR with the wrong location.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
The callback which generates the body of the outlined function uses the correct location.
This was referenced Jul 1, 2025
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.
This is combination of #138149 and #138039 which were opened separately for ease of reviewing. Only other change is adjustments in 2 tests which have gone in since.
There are
DeclareOppresent for the variables mapped into target region. That allow us to generate debug information for them. But theTargetOpis still part of parent function and those variables get the parent function'sDISubprogramas a scope.In
OMPIRBuilder, a new function is created for theTargetOp. We also create a newDISubprogramfor it. All the variables that were in the target region now have to be updated to have the correct scope. This after the fact updating ofdebug information becomes very difficult in certain cases. Take the example of variable arrays. The type of those arrays depend on the artificial
DILocalVariable(s) which hold the size(s) of the array. This new function will now require that we generate the new variable and and new types. Similar issue exist for character type variables too.To avoid this after the fact updating, this PR generates a
DISubprogramAttrfor theTargetOpwhile generating the debug info inflang. Then we don't need to generate aDISubprograminOMPIRBuilder. This change is made a bit more complicated by the the fact that in new scheme, the debug location already points to the newDISubprogramby the time it reachesconvertOmpTarget. But we need some code generation in the parent function so we have to carefully manage the debug locations.This fixes issue
#134991.