Skip to content
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

[InstCombine][DebugInfo] Update debug info after inverting a boolean instruction #71212

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Nov 3, 2023

This PR adapts all dbg users after inverting a boolean instruction.

Related issue: #71065.
After this patch, gdb/lldb prints 255 for l_4516 in the original code. It looks like the DWARF expression is incorrect. I will fix it in the follow-up PR.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 3, 2023

@llvm/pr-subscribers-debuginfo

Author: Yingwei Zheng (dtcxzyw)

Changes

This PR adapts all dbg users after inverting a boolean instruction.

Related issue: #71065.
After this patch, gdb/lldb prints 255 for l_4516 in the original code. It looks like the DWARF expression is incorrect. I will fix it in the follow-up PR.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+7)
  • (added) llvm/test/DebugInfo/salvage-invert-bool.ll (+35)
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 559eb2ef4795eb1..287b7711223901c 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -1187,6 +1187,13 @@ void InstCombinerImpl::freelyInvertAllUsersOf(Value *I, Value *IgnoredUser) {
                        "canFreelyInvertAllUsersOf() ?");
     }
   }
+
+  // Adapt all dbg users
+  if (auto *Inst = dyn_cast<Instruction>(I); Inst && Inst->isUsedByMetadata()) {
+    auto *Not = Builder.CreateNot(I);
+    auto *NotInst = dyn_cast<Instruction>(I);
+    replaceAllDbgUsesWith(*Inst, *Not, NotInst ? *NotInst : *Inst, DT);
+  }
 }
 
 /// Given a 'sub' instruction, return the RHS of the instruction if the LHS is a
diff --git a/llvm/test/DebugInfo/salvage-invert-bool.ll b/llvm/test/DebugInfo/salvage-invert-bool.ll
new file mode 100644
index 000000000000000..8aaee919a2736a3
--- /dev/null
+++ b/llvm/test/DebugInfo/salvage-invert-bool.ll
@@ -0,0 +1,35 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
+; RUN: opt %s -passes=instcombine -S | FileCheck %s
+
+define i32 @test_invert_bool(i32 %call, i32 %a) {
+; CHECK-LABEL: define i32 @test_invert_bool(
+; CHECK-SAME: i32 [[CALL:%.*]], i32 [[A:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TOBOOL_NOT:%.*]] = icmp eq i32 [[CALL]], 0
+; CHECK-NEXT:    call void @llvm.dbg.value(metadata i1 [[TOBOOL_NOT]], metadata [[META3:![0-9]+]], metadata !DIExpression(DW_OP_constu, 18446744073709551615, DW_OP_xor, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value)), !dbg [[DBG8:![0-9]+]]
+; CHECK-NEXT:    [[TMP0:%.*]] = and i32 [[A]], 1
+; CHECK-NEXT:    [[AND:%.*]] = select i1 [[TOBOOL_NOT]], i32 0, i32 [[TMP0]]
+; CHECK-NEXT:    ret i32 [[AND]]
+;
+entry:
+  %tobool = icmp ne i32 %call, 0
+  %land.ext = zext i1 %tobool to i32
+  call void @llvm.dbg.value(metadata i32 %land.ext, metadata !11, metadata !DIExpression()), !dbg !16
+  %and = and i32 %land.ext, %a
+  ret i32 %and
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!10}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/")
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!10 = !{i32 2, !"Debug Info Version", i32 3}
+!11 = !DILocalVariable(name: "var", scope: !12, type: !5)
+!12 = distinct !DISubprogram(name: "test", scope: !1, file: !1, type: !13, unit: !0)
+!13 = !DISubroutineType(types: !14)
+!14 = !{null}
+!16 = !DILocation(scope: !12)

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 3, 2023

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

This PR adapts all dbg users after inverting a boolean instruction.

Related issue: #71065.
After this patch, gdb/lldb prints 255 for l_4516 in the original code. It looks like the DWARF expression is incorrect. I will fix it in the follow-up PR.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+7)
  • (added) llvm/test/DebugInfo/salvage-invert-bool.ll (+35)
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 559eb2ef4795eb1..287b7711223901c 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -1187,6 +1187,13 @@ void InstCombinerImpl::freelyInvertAllUsersOf(Value *I, Value *IgnoredUser) {
                        "canFreelyInvertAllUsersOf() ?");
     }
   }
+
+  // Adapt all dbg users
+  if (auto *Inst = dyn_cast<Instruction>(I); Inst && Inst->isUsedByMetadata()) {
+    auto *Not = Builder.CreateNot(I);
+    auto *NotInst = dyn_cast<Instruction>(I);
+    replaceAllDbgUsesWith(*Inst, *Not, NotInst ? *NotInst : *Inst, DT);
+  }
 }
 
 /// Given a 'sub' instruction, return the RHS of the instruction if the LHS is a
diff --git a/llvm/test/DebugInfo/salvage-invert-bool.ll b/llvm/test/DebugInfo/salvage-invert-bool.ll
new file mode 100644
index 000000000000000..8aaee919a2736a3
--- /dev/null
+++ b/llvm/test/DebugInfo/salvage-invert-bool.ll
@@ -0,0 +1,35 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
+; RUN: opt %s -passes=instcombine -S | FileCheck %s
+
+define i32 @test_invert_bool(i32 %call, i32 %a) {
+; CHECK-LABEL: define i32 @test_invert_bool(
+; CHECK-SAME: i32 [[CALL:%.*]], i32 [[A:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TOBOOL_NOT:%.*]] = icmp eq i32 [[CALL]], 0
+; CHECK-NEXT:    call void @llvm.dbg.value(metadata i1 [[TOBOOL_NOT]], metadata [[META3:![0-9]+]], metadata !DIExpression(DW_OP_constu, 18446744073709551615, DW_OP_xor, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value)), !dbg [[DBG8:![0-9]+]]
+; CHECK-NEXT:    [[TMP0:%.*]] = and i32 [[A]], 1
+; CHECK-NEXT:    [[AND:%.*]] = select i1 [[TOBOOL_NOT]], i32 0, i32 [[TMP0]]
+; CHECK-NEXT:    ret i32 [[AND]]
+;
+entry:
+  %tobool = icmp ne i32 %call, 0
+  %land.ext = zext i1 %tobool to i32
+  call void @llvm.dbg.value(metadata i32 %land.ext, metadata !11, metadata !DIExpression()), !dbg !16
+  %and = and i32 %land.ext, %a
+  ret i32 %and
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!10}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/")
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!10 = !{i32 2, !"Debug Info Version", i32 3}
+!11 = !DILocalVariable(name: "var", scope: !12, type: !5)
+!12 = distinct !DISubprogram(name: "test", scope: !1, file: !1, type: !13, unit: !0)
+!13 = !DISubroutineType(types: !14)
+!14 = !{null}
+!16 = !DILocation(scope: !12)


// Adapt all dbg users
if (auto *Inst = dyn_cast<Instruction>(I); Inst && Inst->isUsedByMetadata()) {
auto *Not = Builder.CreateNot(I);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we can have debug info create extra instructions, that can lead to codegen variations if debug info is enabled.

I think this needs to be encoded in a DWARF expression, not an IR instruction.

@jmorse is more expert at DWARF location information, though

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's right, the presence of debug info may never affect the generated code. You probably want to create a DIExpression that has a DW_OP_not (and potentially modify the Verifier to allow this if this is the first time it's being introduced).

@dtcxzyw dtcxzyw requested a review from jmorse November 4, 2023 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants