[flang][debug] Generate DISubprogramAttr for omp::TargetOp.

[abidh]

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 PR is flang side of the change. The #138149 makes the required changes in OMPIRBuilder.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

Author: Abid Qadeer (abidh)

Changes

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 PR is flang side of the change. I will open another PR which will make the required changes in OMPIRBuilder.

---

Full diff: https://github.com/llvm/llvm-project/pull/138039.diff

3 Files Affected:

• (modified) flang/lib/Optimizer/Transforms/AddDebugInfo.cpp (+98-1)
• (added) flang/test/Transforms/debug-omp-target-op-1.fir (+35)
• (added) flang/test/Transforms/debug-omp-target-op-2.fir (+53)

diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index c479c1a0892b5..8e7ae4383bfdc 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -34,6 +34,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"
 
@@ -103,6 +104,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 artifical 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(
@@ -510,8 +542,73 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
       subTypeAttr, entities, /*annotations=*/{});
   funcOp->setLoc(builder.getFusedLoc({l}, spAttr));
 
+  /* 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 recId =
+        mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+    mlir::DistinctAttr Id =
+        mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+    llvm::SmallVector<mlir::LLVM::DITypeAttr> types;
+    types.push_back(mlir::LLVM::DINullTypeAttr::get(context));
+    mlir::LLVM::DISubroutineTypeAttr spTy =
+        mlir::LLVM::DISubroutineTypeAttr::get(context, CC, types);
+    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 in copied from the
+    // parent 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));
+  });
+
   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..bb586cdf6e9ab
--- /dev/null
+++ b/flang/test/Transforms/debug-omp-target-op-1.fir
@@ -0,0 +1,35 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+
+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]]>
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"{{.*}}>

[abidh]

Polite ping.

[skatrak]

Thank you Abid, can you check buildbot failures? They seem to be related to the patch.

[abidh]

Thank you Abid, can you check buildbot failures? They seem to be related to the patch.
Hi Sergio,
Thanks for having a look. The failures are expected as this PR needs the accompanying #138149 for build/tests to work correctly. I kept them separate for ease of reviewing. Once both of them are approved, I will bring them together before merge to make sure that build bots are green and they go in as one commit.

[skatrak]

This seems fine to me, I just have a couple of nits and potentially basic questions. Thanks again!

[skatrak]

I can see above that for a regular function, the list of types includes one element per result (or a FINullTypeAttr, if there are none, like here), but it also includes one element per function argument. Shouldn't we be adding them here as well, based on whichever map-like arguments the omp.target op has?

[abidh]

Thanks for pointing that out. The type of the function did not matter that much for the final DWARF that came out but I have now added the types from the arguments to the TargetOp for completeness.

[skatrak]

In this approach, the argument types correspond to the entry block arguments of the omp.target operation, and my understanding is that this list of types is intended to correspond to the argument list of the outlined function later generated for that target region. Let me know if I'm wrong about that.

If that is the case, I believe that this wouldn't be the correct set of types. If we look at createOutlinedFunction in the OMPIRBuilder, we can see that for the target device an extra pointer argument is added (which here we aren't doing here, but instead fixing it up at the end of that codegen function) and the rest of the argument list is passed by the caller. Tracking down callers, this eventually corresponds to the Inputs arguments for OpenMPIRBuilder::createTarget, which in OpenMP to LLVM IR translation corresponds to the kernelInput vector in convertOmpTarget. Looking at how it's initialized it's clear that the mapping between omp.target op and its associated outlined LLVM IR function is not so straightforward.

Perhaps the easiest way to deal with this would be to use some placeholder type early on and then update it during codegen, when we have access to the full list of arguments. This hopefully seems like it should not be an issue, since that's what's already done for the extra argument added for the device. Otherwise, we'd have to somehow replicate part of the map-related logic used to populate the argument list here.

[abidh]

As I said above, we don't need the correct function type for the correct dwarf to come out. At the moment, we generate an empty function type in createOutlinedFunction and it works ok. Important part is that we generate the DILocalVariable with ArgNo field set correctly.

[skatrak]

In that case, I'd suggest just passing a single DINullTypeAttr (as if it was a parameter-less void function) and then adding a comment explaining what you're saying, basically stating that figuring out the actual argument list of the outlined function at that point is non-trivial and we don't actually need the function type information to be correct to produce the right dwarf. I think this was your original approach, minus the comment, so sorry about the back and forth before agreeing with you! 😄

That way we don't do any work to fill out the argument types incorrectly anyways, and we keep this decision documented in case we later need to actually fill the argument types properly.

[skatrak]

I wonder whether it's actually necessary to create new attributes to hold the same constant value. I might be wrong on that, just thinking that MLIR attributes are already uniqued at the context level, so my impression is that this would just end up pointing to the same thing in the end. But I may have misunderstood that, so not a request to change anything.

[abidh]

I will do further investigation on it because the similar code is used in other places too.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[abidh]

Polite ping.

[skatrak]

Thank you Abid, and excuse me for the delay giving your changes another look. This mostly LGTM, I just have one remaining concern.

[skatrak]

LGTM, thank you!