Skip to content

[DEBUGINFO] Propagate debug metadata for sext SDNode. #135971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 2, 2025

Conversation

apeskov
Copy link
Contributor

@apeskov apeskov commented Apr 16, 2025

In some cases of chained sext operators the debug metadata can be missed. This patch propagates proper metadata to resulting node.

Particular case of issue is NVPTX codegen for function with bool local variable:

void test(int i) {
  bool xyz = i == 0;
  foo(i);
}

@llvmbot
Copy link
Member

llvmbot commented Apr 16, 2025

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-nvptx

Author: Alexander Peskov (apeskov)

Changes

In some cases of chained sext operators the debug metadata can be missed. This patch propagates proper metadata to resulting node.

Particular case of issue is NVPTX codegen for function with bool local variable:

void test(int i) {
  bool xyz = i == 0;
  foo(i);
}

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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+3-1)
  • (added) llvm/test/DebugInfo/NVPTX/debug-bool-var.ll (+43)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 8682c40898046..c5825a64c1c18 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -6244,7 +6244,9 @@ SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
       SDNodeFlags Flags;
       if (OpOpcode == ISD::ZERO_EXTEND)
         Flags.setNonNeg(N1->getFlags().hasNonNeg());
-      return getNode(OpOpcode, DL, VT, N1.getOperand(0), Flags);
+      SDValue NewVal = getNode(OpOpcode, DL, VT, N1.getOperand(0), Flags);
+      transferDbgValues(N1, NewVal);
+      return NewVal;
     }
     if (N1.isUndef())
       // sext(undef) = 0, because the top bits will all be the same.
diff --git a/llvm/test/DebugInfo/NVPTX/debug-bool-var.ll b/llvm/test/DebugInfo/NVPTX/debug-bool-var.ll
new file mode 100644
index 0000000000000..7e50f1ea18fea
--- /dev/null
+++ b/llvm/test/DebugInfo/NVPTX/debug-bool-var.ll
@@ -0,0 +1,43 @@
+; RUN: llc < %s -mtriple=nvptx64-nvidia-cuda | FileCheck %s
+
+declare void @foo(i32)
+
+define void @test1(i32 noundef %gid) !dbg !3 {
+entry:
+  ;
+  ; verify that debug info exists for "xyz" variable
+  ;
+  ; CHECK-LABEL:    DW_TAG_variable
+  ; CHECK:      .b8 120     // DW_AT_name
+  ; CHECK-NEXT: .b8 121
+  ; CHECK-NEXT: .b8 122
+  ; CHECK-NEXT: .b8 0
+  ; CHECK-NEXT: .b8 1       // DW_AT_decl_file
+  ; CHECK-NEXT: .b8 6       // DW_AT_decl_line
+  ;
+  %cmp = icmp eq i32 %gid, 0, !dbg !12
+  %conv = zext i1 %cmp to i32, !dbg !12
+  %conv1 = trunc i32 %conv to i8, !dbg !12
+    #dbg_value(i8 %conv1, !10, !DIExpression(), !13)
+  %conv3 = sext i8 %conv1 to i32
+  call void @foo(i32 %conv3)
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "test.cu", directory: "/source/dir")
+!2 = !{i32 1, !"Debug Info Version", i32 3}
+!3 = distinct !DISubprogram(name: "test1", linkageName: "_test1i", scope: !1, file: !1, line: 5, type: !4, scopeLine: 5, unit: !0, retainedNodes: !8)
+!4 = !DISubroutineType(types: !5)
+!5 = !{!6, !7}
+!6 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "void")
+!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!8 = !{}
+!9 = distinct !DILexicalBlock(scope: !3, file: !1, line: 5, column: 30)
+!10 = !DILocalVariable(name: "xyz", scope: !9, file: !1, line: 6, type: !11)
+!11 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!12 = !DILocation(line: 1, column: 3, scope: !9)
+!13 = !DILocation(line: 2, scope: !9)

@llvmbot
Copy link
Member

llvmbot commented Apr 16, 2025

@llvm/pr-subscribers-debuginfo

Author: Alexander Peskov (apeskov)

Changes

In some cases of chained sext operators the debug metadata can be missed. This patch propagates proper metadata to resulting node.

Particular case of issue is NVPTX codegen for function with bool local variable:

void test(int i) {
  bool xyz = i == 0;
  foo(i);
}

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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+3-1)
  • (added) llvm/test/DebugInfo/NVPTX/debug-bool-var.ll (+43)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 8682c40898046..c5825a64c1c18 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -6244,7 +6244,9 @@ SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
       SDNodeFlags Flags;
       if (OpOpcode == ISD::ZERO_EXTEND)
         Flags.setNonNeg(N1->getFlags().hasNonNeg());
-      return getNode(OpOpcode, DL, VT, N1.getOperand(0), Flags);
+      SDValue NewVal = getNode(OpOpcode, DL, VT, N1.getOperand(0), Flags);
+      transferDbgValues(N1, NewVal);
+      return NewVal;
     }
     if (N1.isUndef())
       // sext(undef) = 0, because the top bits will all be the same.
diff --git a/llvm/test/DebugInfo/NVPTX/debug-bool-var.ll b/llvm/test/DebugInfo/NVPTX/debug-bool-var.ll
new file mode 100644
index 0000000000000..7e50f1ea18fea
--- /dev/null
+++ b/llvm/test/DebugInfo/NVPTX/debug-bool-var.ll
@@ -0,0 +1,43 @@
+; RUN: llc < %s -mtriple=nvptx64-nvidia-cuda | FileCheck %s
+
+declare void @foo(i32)
+
+define void @test1(i32 noundef %gid) !dbg !3 {
+entry:
+  ;
+  ; verify that debug info exists for "xyz" variable
+  ;
+  ; CHECK-LABEL:    DW_TAG_variable
+  ; CHECK:      .b8 120     // DW_AT_name
+  ; CHECK-NEXT: .b8 121
+  ; CHECK-NEXT: .b8 122
+  ; CHECK-NEXT: .b8 0
+  ; CHECK-NEXT: .b8 1       // DW_AT_decl_file
+  ; CHECK-NEXT: .b8 6       // DW_AT_decl_line
+  ;
+  %cmp = icmp eq i32 %gid, 0, !dbg !12
+  %conv = zext i1 %cmp to i32, !dbg !12
+  %conv1 = trunc i32 %conv to i8, !dbg !12
+    #dbg_value(i8 %conv1, !10, !DIExpression(), !13)
+  %conv3 = sext i8 %conv1 to i32
+  call void @foo(i32 %conv3)
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "test.cu", directory: "/source/dir")
+!2 = !{i32 1, !"Debug Info Version", i32 3}
+!3 = distinct !DISubprogram(name: "test1", linkageName: "_test1i", scope: !1, file: !1, line: 5, type: !4, scopeLine: 5, unit: !0, retainedNodes: !8)
+!4 = !DISubroutineType(types: !5)
+!5 = !{!6, !7}
+!6 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "void")
+!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!8 = !{}
+!9 = distinct !DILexicalBlock(scope: !3, file: !1, line: 5, column: 30)
+!10 = !DILocalVariable(name: "xyz", scope: !9, file: !1, line: 6, type: !11)
+!11 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!12 = !DILocation(line: 1, column: 3, scope: !9)
+!13 = !DILocation(line: 2, scope: !9)

Copy link
Contributor

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this LGTM, just one nit on the test from me.

Unrelated to your change - does this problem occur a few lines down in the if (OpOpcode == ISD::ZERO_EXTEND) { // (zext (zext x)) -> (zext x) block too?

@apeskov apeskov force-pushed the ap/debug-info-propagation-ext-node branch from 3e325b9 to 0f7477b Compare April 16, 2025 16:36
Copy link
Contributor

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually - could this create incorrect locations, if the sddbgvalue was attached to a zext but with this change gets moved onto a sext?

@apeskov apeskov force-pushed the ap/debug-info-propagation-ext-node branch from 0f7477b to 0f34707 Compare April 17, 2025 12:50
Copy link

github-actions bot commented Apr 17, 2025

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

@apeskov
Copy link
Contributor Author

apeskov commented Apr 17, 2025

does this problem occur a few lines down?

Yes, it does. I extended test to cover zext-zext case as well. I checkd, frontend generates similar kind of IR in case of unsigned char local variable.

just one nit on the test from me.

I don't see any nit comments about test itself. Perhaps you forgot press "submit" button for review.

Copy link
Contributor

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. I extended test to cover zext-zext case as well. I checkd, frontend generates similar kind of IR in case of unsigned char local variable.

Thanks!

I don't see any nit comments about test itself. Perhaps you forgot press "submit" button for review.

Oops. Probably - I've added it now.

Actually - could this create incorrect locations, if the sddbgvalue was attached to a zext but with this change gets moved onto a sext?

Looking again at my question - the new SDNode is created with the OpOpcode type, so that's not a concern. We only go sext (sext x) ->sext x or sext (zext x) ->zext x, never sext (zext x) -> sext x. Right?

@@ -0,0 +1,84 @@
; RUN: llc < %s -mtriple=nvptx64-nvidia-cuda | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use -print-after=finalize-isel to reduce the surface area being tested (and it'd make the output slightly easier to read IMO)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Test is reworded with "finalize-isel".

@apeskov apeskov force-pushed the ap/debug-info-propagation-ext-node branch from 54d98c9 to d61d118 Compare May 2, 2025 09:11
@apeskov
Copy link
Contributor Author

apeskov commented May 2, 2025

We only go sext (sext x) ->sext x or sext (zext x) ->zext x, never sext (zext x) -> sext x. Right?

You are right, sext (zext x) -> sext x is not the case. In other words, debug data is only transferred between nodes with identical opcode.

Copy link
Contributor

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, this LGTM (with a final nit, but feel free to merge once that's fixed). Thanks!

@enferex enferex merged commit cffb8ae into llvm:main May 2, 2025
11 checks passed
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
In some cases of chained `sext` operators the debug metadata can be
missed. This patch propagates proper metadata to resulting node.

Particular case of issue is NVPTX codegen for function with bool local
variable:
```
void test(int i) {
  bool xyz = i == 0;
  foo(i);
}
```

---------

Signed-off-by: Alexander Peskov <[email protected]>
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
In some cases of chained `sext` operators the debug metadata can be
missed. This patch propagates proper metadata to resulting node.

Particular case of issue is NVPTX codegen for function with bool local
variable:
```
void test(int i) {
  bool xyz = i == 0;
  foo(i);
}
```

---------

Signed-off-by: Alexander Peskov <[email protected]>
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
In some cases of chained `sext` operators the debug metadata can be
missed. This patch propagates proper metadata to resulting node.

Particular case of issue is NVPTX codegen for function with bool local
variable:
```
void test(int i) {
  bool xyz = i == 0;
  foo(i);
}
```

---------

Signed-off-by: Alexander Peskov <[email protected]>
GeorgeARM pushed a commit to GeorgeARM/llvm-project that referenced this pull request May 7, 2025
In some cases of chained `sext` operators the debug metadata can be
missed. This patch propagates proper metadata to resulting node.

Particular case of issue is NVPTX codegen for function with bool local
variable:
```
void test(int i) {
  bool xyz = i == 0;
  foo(i);
}
```

---------

Signed-off-by: Alexander Peskov <[email protected]>
@apeskov apeskov deleted the ap/debug-info-propagation-ext-node branch May 15, 2025 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants