[OpenMPIRBuilder] Don't drop debug info for target region. #80692

abidh · 2024-02-05T15:08:55Z

When an outlined function is generated for omp target region, a corresponding DISubprogram was not being generated. This resulted in all the debug information for the target region being dropped.

This commit adds DISubprogram for the outlined function if there is one available for the parent function. It also updates the current debug location so that the right scope is used for the entries in the outlined function.

With this change in place, I can set source line breakpoint in target region and run to them in debugger.

llvmbot · 2024-02-05T15:09:25Z

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

@llvm/pr-subscribers-flang-openmp

Author: None (abidh)

Changes

When an outlined function is generated for omp target region, a corresponding DISubprogram was not being generated. This resulted in all the debug information for the target region being dropped.

This commit adds DISubprogram for the outlined function if there is one available for the parent function. It also updates the current debug location so that the right scope is used for the entries in the outlined function.

With this change in place, I can set source line breakpoint in target region and run to them in debugger.

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

3 Files Affected:

(added) flang/test/Lower/OpenMP/target-debug.f90 (+18)
(modified) llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp (+34-2)
(modified) llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp (+4)

diff --git a/flang/test/Lower/OpenMP/target-debug.f90 b/flang/test/Lower/OpenMP/target-debug.f90
new file mode 100644
index 0000000000000..c9843eba391d7
--- /dev/null
+++ b/flang/test/Lower/OpenMP/target-debug.f90
@@ -0,0 +1,18 @@
+!RUN: %flang_fc1 -triple amdgcn-amd-amdhsa %s -debug-info-kind=line-tables-only -fopenmp -fopenmp-is-target-device -emit-llvm -o - | FileCheck %s
+program test
+implicit none
+
+  integer(kind = 4) :: a, b, c, d
+
+  !$omp target map(tofrom: a, b, c, d)
+  a = a + 1
+  ! CHECK: !DILocation(line: [[@LINE-1]]
+  b = a + 2
+  ! CHECK: !DILocation(line: [[@LINE-1]]
+  c = a + 3
+  ! CHECK: !DILocation(line: [[@LINE-1]]
+  d = a + 4
+  ! CHECK: !DILocation(line: [[@LINE-1]]
+  !$omp end target
+
+end program test
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 02b333e9ccd56..f0f26c5b8d8b7 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -31,6 +31,7 @@
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
@@ -5025,10 +5026,41 @@ static Function *createOutlinedFunction(
       ParameterTypes.push_back(Arg->getType());
   }
 
+  auto BB = Builder.GetInsertBlock();
+  auto M = BB->getModule();
   auto FuncType = FunctionType::get(Builder.getVoidTy(), ParameterTypes,
                                     /*isVarArg*/ false);
-  auto Func = Function::Create(FuncType, GlobalValue::InternalLinkage, FuncName,
-                               Builder.GetInsertBlock()->getModule());
+  auto Func =
+      Function::Create(FuncType, GlobalValue::InternalLinkage, FuncName, M);
+
+  // 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();
+      // 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(std::nullopt));
+      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()));
+    }
+  }
 
   // Save insert point.
   auto OldInsertPoint = Builder.saveIP();
diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index e79d0bb2f65ae..c5b5d12840c4d 100644
--- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -5827,6 +5827,10 @@ TEST_F(OpenMPIRBuilderTest, TargetRegion) {
   BasicBlock *FallbackBlock = Branch->getSuccessor(0);
   Iter = FallbackBlock->rbegin();
   CallInst *FCall = dyn_cast<CallInst>(&*(++Iter));
+  // 'F' has a dummy DISubprogram which causes OutlinedFunc to also
+  // have a DISubprogram. In this case, the call to OutlinedFunc needs
+  // to have a debug loc, otherwise verifier will complain.
+  FCall->setDebugLoc(DL);
   EXPECT_NE(FCall, nullptr);
 
   // Check that the correct aguments are passed in

DominikAdamski

Is it possible to generate MLIR file and

DominikAdamski · 2024-02-06T07:55:43Z

flang/test/Lower/OpenMP/target-debug.f90

Is it possible to generate MLIR file and add test for LLVM IR here ?

jdoerfert · 2024-02-06T16:31:09Z

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

+
+  // If there's a DISubprogram associated with current function, then
+  // generate one for the outlined function.
+  if (Function *parentFunc = BB->getParent()) {


Do we expect this to be an unconnected BB at any point?
Here, and below, please use LLVM style. This is not MLIR.

I don't think BB will be unconnected. But even if that was to happen, the worst we will do is to drop debug bits as we are already doing.

Thanks for pointing out the style bit. Corrected now.

abidh · 2024-02-13T12:20:12Z

Ping. Any further review comments for this PR?

abidh · 2024-03-06T17:39:47Z

Ping.

mjklemm · 2024-07-03T11:35:12Z

@jdoerfert @DominikAdamski @kiranchandramohan Kindly requesting to review this PR, so that it can land.

kiranchandramohan

LG.

kiranchandramohan · 2024-07-03T11:58:21Z

mlir/test/Target/LLVMIR/omptarget-debug.mlir

+module attributes {omp.is_target_device = true} {
+  llvm.func @_QQmain() {
+    %0 = llvm.mlir.constant(1 : i64) : i64
+    %1 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr<5>
+    %2 = llvm.addrspacecast %1 : !llvm.ptr<5> to !llvm.ptr
+    %3 = llvm.mlir.constant(1 : i64) : i64
+    %4 = llvm.alloca %3 x i32 : (i64) -> !llvm.ptr<5>
+    %5 = llvm.addrspacecast %4 : !llvm.ptr<5> to !llvm.ptr
+    %6 = llvm.mlir.constant(1 : i64) : i64
+    %7 = llvm.alloca %6 x i32 : (i64) -> !llvm.ptr<5>
+    %8 = llvm.addrspacecast %7 : !llvm.ptr<5> to !llvm.ptr
+    %9 = omp.map_info var_ptr(%2 : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = ""}
+    %10 = omp.map_info var_ptr(%5 : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = ""}
+    omp.target map_entries(%9 -> %arg0, %10 -> %arg1 : !llvm.ptr, !llvm.ptr) {
+    ^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr):
+      %12 = llvm.mlir.constant(2 : i32) : i32
+      %13 = llvm.mlir.constant(1 : i32) : i32
+      %14 = llvm.load %arg0 : !llvm.ptr -> i32 loc(#loc2)
+      %15 = llvm.add %14, %13  : i32 loc(#loc2)
+      llvm.store %15, %arg0 : i32, !llvm.ptr loc(#loc2)
+      %16 = llvm.load %arg0 : !llvm.ptr -> i32 loc(#loc3)
+      %17 = llvm.add %16, %12  : i32 loc(#loc3)
+      llvm.store %17, %arg1 : i32, !llvm.ptr loc(#loc3)
+      omp.terminator
+    }
+    %11 = omp.map_info var_ptr(%8 : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = ""}
+    omp.target map_entries(%11 -> %arg0 : !llvm.ptr) {
+    ^bb0(%arg0: !llvm.ptr):
+      %12 = llvm.mlir.constant(1 : i32) : i32
+      omp.parallel {
+        %13 = llvm.load %arg0 : !llvm.ptr -> i32 loc(#loc4)
+        %14 = llvm.add %13, %12  : i32 loc(#loc4)
+        llvm.store %14, %arg0 : i32, !llvm.ptr loc(#loc4)
+        omp.terminator
+      }
+      omp.terminator
+    }
+    llvm.return
+  } loc(#loc5)
+} loc(#loc)


Nit: can you minimize this testcase to be as small as possible? We do not need a large region particularly.

I have minimized the test. I also had to make one other small change after I rebased. There was debug info for variables that gets passed into the targer region. They can fail LLVM verifier checks as the scope of the variable may not match the function created for target region. This may require extra work so I have disabled the debug info for such variables.

github-actions · 2024-07-09T13:58:59Z

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

abidh · 2024-08-28T20:13:31Z

I have improved the patch a bit more and added some more tests especially to check that variable in target region don't cause a problem. I have tested it further and it seems ready to merge. Please let me know if there are any further comments.

abidh · 2024-09-02T10:29:06Z

@jeanPerier Can you kindly look at the this PR especially the flang changes where I try to skip the debug info generation for variables which will be in OpenMP target region. Is this the right way to do it? I thought about processing DeclOp just in the function entry block which will have the same effect but this seems to make the intention more explicit.

jeanPerier · 2024-09-03T07:22:20Z

flang/lib/Optimizer/Transforms/AddDebugInfo.cpp

+    // FIXME: We currently dont handle variables that are not in the entry
+    // blocks of the fuctions. These may be variable or arguments used in the
+    // OpenMP target regions.
+    if (&funcOp.front() == declOp->getBlock())


These can also be variables from Fortran BLOCK constructs, or the selectors from ASSOCIATE/SELECT TYPE/SELECT RANK/SELECT CASE.

I think it is fine to do it the way you are doing given we will want to lift this restriction, so any compiler performance gain would be temporary and artificial (it would otherwise be more efficient to walk only the front block).

When an outlined function is generated for omp target region, a corresponding DISubprogram was not being generated. This resulted in all the debug information for the target region being dropped. This commit adds DISubprogram for the outlined function if there is one available for the parent function. It also updates the current debug location so that the right scope is used for the entries in the outlined function. With this change in place, I can set source line breakpoint in target region and run to them in debugger.

The original f90 ->llvmir test has been removed. I have added an MLIR -> llvmir test as per review comments.

These variables currently fail the llvm verifier tests as the scope inside the variable is still point to the parent function of the target region.

It has the extra benefit of saving/restoring the debug location which otherwise could be lost.

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp clang:openmp OpenMP related changes to Clang labels Feb 5, 2024

abidh requested review from jsjodin, kiranchandramohan and agozillon February 5, 2024 15:10

DominikAdamski reviewed Feb 6, 2024

View reviewed changes

jdoerfert reviewed Feb 6, 2024

View reviewed changes

llvmbot added mlir:llvm mlir labels Feb 7, 2024

abidh requested review from jdoerfert and DominikAdamski February 28, 2024 15:57

kiranchandramohan approved these changes Jul 3, 2024

View reviewed changes

abidh force-pushed the target_debug branch from a989a44 to 3450d4b Compare July 9, 2024 13:54

abidh force-pushed the target_debug branch from a6eea44 to 7034ffc Compare July 11, 2024 07:11

abidh force-pushed the target_debug branch 3 times, most recently from 24cbf9e to a780f18 Compare August 28, 2024 16:27

abidh requested a review from jeanPerier September 2, 2024 10:25

jeanPerier approved these changes Sep 3, 2024

View reviewed changes

abidh added 2 commits September 3, 2024 10:26

Move the test into MLIR.

9978dc3

The original f90 ->llvmir test has been removed. I have added an MLIR -> llvmir test as per review comments.

abidh added 8 commits September 3, 2024 10:26

Correct the case on variable name.

da3ac09

Minimize testcase as per review comments.

d3585a8

Don't generate debug info for variable in target region.

f1b4e54

These variables currently fail the llvm verifier tests as the scope inside the variable is still point to the parent function of the target region.

Improve test case.

4d1507a

Use InsertPointGuard instead of manual save/restore.

9370c29

It has the extra benefit of saving/restoring the debug location which otherwise could be lost.

Check DebugLoc is valid before using it.

a4b6b77

Add testcase with is_target_device = false

beda51c

Avoid using stale debug location.

bab8185

abidh force-pushed the target_debug branch from a780f18 to bab8185 Compare September 3, 2024 10:46

abidh merged commit 9e08db7 into llvm:main Sep 4, 2024
8 checks passed

abidh deleted the target_debug branch September 11, 2024 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OpenMPIRBuilder] Don't drop debug info for target region. #80692

[OpenMPIRBuilder] Don't drop debug info for target region. #80692

abidh commented Feb 5, 2024

llvmbot commented Feb 5, 2024 •

edited

Loading

DominikAdamski left a comment

DominikAdamski Feb 6, 2024

abidh Feb 7, 2024

jdoerfert Feb 6, 2024

abidh Feb 7, 2024

abidh commented Feb 13, 2024

abidh commented Mar 6, 2024

mjklemm commented Jul 3, 2024

kiranchandramohan left a comment

kiranchandramohan Jul 3, 2024

abidh Jul 9, 2024

github-actions bot commented Jul 9, 2024 •

edited

Loading

abidh commented Aug 28, 2024

abidh commented Sep 2, 2024

jeanPerier Sep 3, 2024

jeanPerier Sep 3, 2024

[OpenMPIRBuilder] Don't drop debug info for target region. #80692

[OpenMPIRBuilder] Don't drop debug info for target region. #80692

Conversation

abidh commented Feb 5, 2024

llvmbot commented Feb 5, 2024 • edited Loading

DominikAdamski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidh commented Feb 13, 2024

abidh commented Mar 6, 2024

mjklemm commented Jul 3, 2024

kiranchandramohan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 9, 2024 • edited Loading

abidh commented Aug 28, 2024

abidh commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvmbot commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading