[llvm][DebugInfo] Emit 0/1 for constant boolean values #151225
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-backend-nvptx Author: Laxman (laxmansole) ChangesFor boolean values, -1 is assigned to indicate This leads to the debugger printing 255 instead of "true" for boolean variables. This change emits Full diff: https://github.com/llvm/llvm-project/pull/151225.diff 6 Files Affected:
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index 5577a7d298177..f1be286c0a746 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -247,6 +247,13 @@ void DwarfCompileUnit::addLocationAttribute(
DIELoc *Loc = nullptr;
std::optional<unsigned> NVPTXAddressSpace;
std::unique_ptr<DIEDwarfExpression> DwarfExpr;
+
+ // Check if this variable is of boolean type
+ bool isBoolean = false;
+ if (GV && GV->getType())
+ if (auto *BasicType = dyn_cast<DIBasicType>(GV->getType()))
+ isBoolean = BasicType->getEncoding() == dwarf::DW_ATE_boolean;
+
for (const auto &GE : GlobalExprs) {
const GlobalVariable *Global = GE.Var;
const DIExpression *Expr = GE.Expr;
@@ -257,11 +264,17 @@ void DwarfCompileUnit::addLocationAttribute(
// DW_AT_const_value(X).
if (GlobalExprs.size() == 1 && Expr && Expr->isConstant()) {
addToAccelTable = true;
+
+ // Determine the value to use, normalizing booleans to 0 or 1
+ int64_t valueToUse = Expr->getElement(1);
+ if (isBoolean)
+ valueToUse = valueToUse ? 1 : 0;
+
addConstantValue(
*VariableDIE,
DIExpression::SignedOrUnsignedConstant::UnsignedConstant ==
*Expr->isConstant(),
- Expr->getElement(1));
+ valueToUse);
break;
}
@@ -820,6 +833,22 @@ void DwarfCompileUnit::applyConcreteDbgVariableAttributes(
}
if (!DVal->isVariadic()) {
const DbgValueLocEntry *Entry = DVal->getLocEntries().begin();
+
+ // Helper function to handle boolean constant values with type safety
+ auto addConstantValueWithBooleanNormalization =
+ [&](DIE &VariableDie, uint64_t intValue, const DIType *Type) {
+ if (auto *BasicType = dyn_cast_or_null<DIBasicType>(Type)) {
+ if (BasicType->getEncoding() == dwarf::DW_ATE_boolean) {
+ // Normalize boolean values: any non-zero becomes 1, zero stays 0
+ uint64_t normalizedBoolValue = (intValue) ? 1 : 0;
+ addConstantValue(VariableDie, normalizedBoolValue, Type);
+ return;
+ }
+ }
+ // For non-boolean types, use the original constant value
+ addConstantValue(VariableDie, intValue, Type);
+ };
+
if (Entry->isLocation()) {
addVariableAddress(DV, VariableDie, Entry->getLoc());
} else if (Entry->isInt()) {
@@ -836,7 +865,8 @@ void DwarfCompileUnit::applyConcreteDbgVariableAttributes(
addUInt(VariableDie, dwarf::DW_AT_LLVM_tag_offset,
dwarf::DW_FORM_data1, *DwarfExpr.TagOffset);
} else
- addConstantValue(VariableDie, Entry->getInt(), DV.getType());
+ addConstantValueWithBooleanNormalization(VariableDie, Entry->getInt(),
+ DV.getType());
} else if (Entry->isConstantFP()) {
addConstantFPValue(VariableDie, Entry->getConstantFP());
} else if (Entry->isConstantInt()) {
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 71888332a6620..6d4c9c05e3aba 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -3100,8 +3100,10 @@ void DwarfDebug::emitDebugLocValue(const AsmPrinter &AP, const DIBasicType *BT,
&AP](const DbgValueLocEntry &Entry,
DIExpressionCursor &Cursor) -> bool {
if (Entry.isInt()) {
- if (BT && (BT->getEncoding() == dwarf::DW_ATE_signed ||
- BT->getEncoding() == dwarf::DW_ATE_signed_char))
+ if (BT && (BT->getEncoding() == dwarf::DW_ATE_boolean))
+ DwarfExpr.addBooleanConstant(Entry.getInt());
+ else if (BT && (BT->getEncoding() == dwarf::DW_ATE_signed ||
+ BT->getEncoding() == dwarf::DW_ATE_signed_char))
DwarfExpr.addSignedConstant(Entry.getInt());
else
DwarfExpr.addUnsignedConstant(Entry.getInt());
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
index e684054ffa3e4..8a30714db2fdf 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
@@ -194,6 +194,15 @@ void DwarfExpression::addStackValue() {
emitOp(dwarf::DW_OP_stack_value);
}
+void DwarfExpression::addBooleanConstant(int64_t Value) {
+ assert(isImplicitLocation() || isUnknownLocation());
+ LocationKind = Implicit;
+ if (Value == 0)
+ emitOp(dwarf::DW_OP_lit0);
+ else
+ emitOp(dwarf::DW_OP_lit1);
+}
+
void DwarfExpression::addSignedConstant(int64_t Value) {
assert(isImplicitLocation() || isUnknownLocation());
LocationKind = Implicit;
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
index 06809ab263875..700e0ec5813ee 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
@@ -229,6 +229,9 @@ class DwarfExpression {
/// This needs to be called last to commit any pending changes.
void finalize();
+ /// Emit a boolean constant.
+ void addBooleanConstant(int64_t Value);
+
/// Emit a signed constant.
void addSignedConstant(int64_t Value);
diff --git a/llvm/test/DebugInfo/NVPTX/debug-bool-const-location.ll b/llvm/test/DebugInfo/NVPTX/debug-bool-const-location.ll
new file mode 100644
index 0000000000000..f073d51e7ef99
--- /dev/null
+++ b/llvm/test/DebugInfo/NVPTX/debug-bool-const-location.ll
@@ -0,0 +1,51 @@
+; RUN: llc < %s -asm-verbose -mattr=+ptx76 -O0| FileCheck %s
+; RUN: %if ptxas %{ llc < %s -asm-verbose -mattr=+ptx76 -O0 | %ptxas-verify %}
+
+target triple = "nvptx64-nvidia-cuda"
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-i128:128:128-f32:32:32-f64:64:64-v16:16:16-v32:32:32-v64:64:64-v128:128:128-n16:32:64"
+
+; CHECK: {{.*}}section {{.*}}debug_loc
+; CHECK: .b8 48{{.*}} DW_OP_lit0
+; CHECK: .b8 49{{.*}} DW_OP_lit1
+; CHECK: .b8 144{{.*}} DW_OP_regx
+
+define void @foo(i8 %"arg.arg") !dbg !5
+{
+entry:
+ %".4" = alloca i1
+ %".5" = icmp eq i8 %"arg.arg", 0
+ %arg = alloca i1
+ br i1 %".5", label %"entry.if", label %"entry.else"
+entry.if:
+ store i1 false, i1* %arg
+ call void @"llvm.dbg.value"(metadata i1 false , metadata !9, metadata !10), !dbg !6
+ br label %"entry.endif"
+entry.else:
+ store i1 true, i1* %arg
+ call void @"llvm.dbg.value"(metadata i1 true , metadata !9, metadata !10), !dbg !7
+ br label %"entry.endif"
+entry.endif:
+ %".11" = load i1, i1* %arg
+ store i1 %".11", i1* %".4", !dbg !8
+ call void @"llvm.dbg.value"(metadata i1 %".11" , metadata !9, metadata !10), !dbg !8
+ ret void, !dbg !8
+}
+
+declare void @"llvm.dbg.value"(metadata %".1", metadata %".2", metadata %".3")
+
+!llvm.dbg.cu = !{ !2 }
+!llvm.module.flags = !{ !11, !12 }
+!nvvm.annotations = !{}
+
+!1 = !DIFile(directory: "/source/dir", filename: "test.cu")
+!2 = distinct !DICompileUnit(emissionKind: FullDebug, file: !1, isOptimized: false, language: DW_LANG_C_plus_plus, runtimeVersion: 0)
+!3 = !DIBasicType(encoding: DW_ATE_boolean, name: "bool", size: 8)
+!4 = !DISubroutineType(types: !{null})
+!5 = distinct !DISubprogram(file: !1, isDefinition: true, isLocal: false, isOptimized: false, line: 5, linkageName: "foo", name: "foo", scope: !1, scopeLine: 5, type: !4, unit: !2)
+!6 = !DILocation(column: 1, line: 5, scope: !5)
+!7 = !DILocation(column: 1, line: 7, scope: !5)
+!8 = !DILocation(column: 1, line: 8, scope: !5)
+!9 = !DILocalVariable(arg: 0, file: !1, line: 5, name: "arg", scope: !5, type: !3)
+!10 = !DIExpression()
+!11 = !{ i32 2, !"Dwarf Version", i32 4 }
+!12 = !{ i32 2, !"Debug Info Version", i32 3 }
\ No newline at end of file
diff --git a/llvm/test/DebugInfo/NVPTX/debug-bool-const-value.ll b/llvm/test/DebugInfo/NVPTX/debug-bool-const-value.ll
new file mode 100644
index 0000000000000..002a7a801c746
--- /dev/null
+++ b/llvm/test/DebugInfo/NVPTX/debug-bool-const-value.ll
@@ -0,0 +1,37 @@
+; RUN: llc < %s -asm-verbose -mattr=+ptx76 | FileCheck %s
+; RUN: %if ptxas %{ llc < %s -asm-verbose -mattr=+ptx76 | %ptxas-verify %}
+
+target triple = "nvptx64-nvidia-cuda"
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-i128:128:128-f32:32:32-f64:64:64-v16:16:16-v32:32:32-v64:64:64-v128:128:128-n16:32:64"
+
+; CHECK: {{.*}}section {{.*}}debug_info
+; CHECK: {{.*}}DW_TAG_variable
+; CHECK-NEXT: {{.*}} DW_AT_address_class
+; CHECK-NEXT: .b8 1{{.*}} DW_AT_const_value
+; CHECK-NEXT: {{.*}} DW_AT_name
+
+define void @test() !dbg !5
+{
+entry:
+ %arg = alloca i1
+ store i1 true, i1* %arg, !dbg !6
+ call void @"llvm.dbg.value"(metadata i1 true, metadata !7, metadata !8), !dbg !6
+ ret void, !dbg !6
+}
+
+declare void @"llvm.dbg.value"(metadata %".1", metadata %".2", metadata %".3")
+
+!llvm.dbg.cu = !{ !2 }
+!llvm.module.flags = !{ !9, !10 }
+!nvvm.annotations = !{}
+
+!1 = !DIFile(directory: "/source/dir", filename: "test.cu")
+!2 = distinct !DICompileUnit(emissionKind: FullDebug, file: !1, isOptimized: false, language: DW_LANG_C_plus_plus, runtimeVersion: 0)
+!3 = !DIBasicType(encoding: DW_ATE_boolean, name: "bool", size: 8)
+!4 = !DISubroutineType(types: !{null})
+!5 = distinct !DISubprogram(file: !1, isDefinition: true, isLocal: false, isOptimized: false, line: 5, linkageName: "test", name: "test", scope: !1, scopeLine: 5, type: !4, unit: !2)
+!6 = !DILocation(column: 1, line: 5, scope: !5)
+!7 = !DILocalVariable(arg: 0, file: !1, line: 5, name: "arg", scope: !5, type: !3)
+!8 = !DIExpression()
+!9 = !{ i32 2, !"Dwarf Version", i32 4 }
+!10 = !{ i32 2, !"Debug Info Version", i32 3 }
\ No newline at end of file
|
|
@llvm/pr-subscribers-debuginfo Author: Laxman (laxmansole) ChangesFor boolean values, -1 is assigned to indicate This leads to the debugger printing 255 instead of "true" for boolean variables. This change emits Full diff: https://github.com/llvm/llvm-project/pull/151225.diff 6 Files Affected:
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index 5577a7d298177..f1be286c0a746 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -247,6 +247,13 @@ void DwarfCompileUnit::addLocationAttribute(
DIELoc *Loc = nullptr;
std::optional<unsigned> NVPTXAddressSpace;
std::unique_ptr<DIEDwarfExpression> DwarfExpr;
+
+ // Check if this variable is of boolean type
+ bool isBoolean = false;
+ if (GV && GV->getType())
+ if (auto *BasicType = dyn_cast<DIBasicType>(GV->getType()))
+ isBoolean = BasicType->getEncoding() == dwarf::DW_ATE_boolean;
+
for (const auto &GE : GlobalExprs) {
const GlobalVariable *Global = GE.Var;
const DIExpression *Expr = GE.Expr;
@@ -257,11 +264,17 @@ void DwarfCompileUnit::addLocationAttribute(
// DW_AT_const_value(X).
if (GlobalExprs.size() == 1 && Expr && Expr->isConstant()) {
addToAccelTable = true;
+
+ // Determine the value to use, normalizing booleans to 0 or 1
+ int64_t valueToUse = Expr->getElement(1);
+ if (isBoolean)
+ valueToUse = valueToUse ? 1 : 0;
+
addConstantValue(
*VariableDIE,
DIExpression::SignedOrUnsignedConstant::UnsignedConstant ==
*Expr->isConstant(),
- Expr->getElement(1));
+ valueToUse);
break;
}
@@ -820,6 +833,22 @@ void DwarfCompileUnit::applyConcreteDbgVariableAttributes(
}
if (!DVal->isVariadic()) {
const DbgValueLocEntry *Entry = DVal->getLocEntries().begin();
+
+ // Helper function to handle boolean constant values with type safety
+ auto addConstantValueWithBooleanNormalization =
+ [&](DIE &VariableDie, uint64_t intValue, const DIType *Type) {
+ if (auto *BasicType = dyn_cast_or_null<DIBasicType>(Type)) {
+ if (BasicType->getEncoding() == dwarf::DW_ATE_boolean) {
+ // Normalize boolean values: any non-zero becomes 1, zero stays 0
+ uint64_t normalizedBoolValue = (intValue) ? 1 : 0;
+ addConstantValue(VariableDie, normalizedBoolValue, Type);
+ return;
+ }
+ }
+ // For non-boolean types, use the original constant value
+ addConstantValue(VariableDie, intValue, Type);
+ };
+
if (Entry->isLocation()) {
addVariableAddress(DV, VariableDie, Entry->getLoc());
} else if (Entry->isInt()) {
@@ -836,7 +865,8 @@ void DwarfCompileUnit::applyConcreteDbgVariableAttributes(
addUInt(VariableDie, dwarf::DW_AT_LLVM_tag_offset,
dwarf::DW_FORM_data1, *DwarfExpr.TagOffset);
} else
- addConstantValue(VariableDie, Entry->getInt(), DV.getType());
+ addConstantValueWithBooleanNormalization(VariableDie, Entry->getInt(),
+ DV.getType());
} else if (Entry->isConstantFP()) {
addConstantFPValue(VariableDie, Entry->getConstantFP());
} else if (Entry->isConstantInt()) {
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 71888332a6620..6d4c9c05e3aba 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -3100,8 +3100,10 @@ void DwarfDebug::emitDebugLocValue(const AsmPrinter &AP, const DIBasicType *BT,
&AP](const DbgValueLocEntry &Entry,
DIExpressionCursor &Cursor) -> bool {
if (Entry.isInt()) {
- if (BT && (BT->getEncoding() == dwarf::DW_ATE_signed ||
- BT->getEncoding() == dwarf::DW_ATE_signed_char))
+ if (BT && (BT->getEncoding() == dwarf::DW_ATE_boolean))
+ DwarfExpr.addBooleanConstant(Entry.getInt());
+ else if (BT && (BT->getEncoding() == dwarf::DW_ATE_signed ||
+ BT->getEncoding() == dwarf::DW_ATE_signed_char))
DwarfExpr.addSignedConstant(Entry.getInt());
else
DwarfExpr.addUnsignedConstant(Entry.getInt());
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
index e684054ffa3e4..8a30714db2fdf 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
@@ -194,6 +194,15 @@ void DwarfExpression::addStackValue() {
emitOp(dwarf::DW_OP_stack_value);
}
+void DwarfExpression::addBooleanConstant(int64_t Value) {
+ assert(isImplicitLocation() || isUnknownLocation());
+ LocationKind = Implicit;
+ if (Value == 0)
+ emitOp(dwarf::DW_OP_lit0);
+ else
+ emitOp(dwarf::DW_OP_lit1);
+}
+
void DwarfExpression::addSignedConstant(int64_t Value) {
assert(isImplicitLocation() || isUnknownLocation());
LocationKind = Implicit;
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
index 06809ab263875..700e0ec5813ee 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
@@ -229,6 +229,9 @@ class DwarfExpression {
/// This needs to be called last to commit any pending changes.
void finalize();
+ /// Emit a boolean constant.
+ void addBooleanConstant(int64_t Value);
+
/// Emit a signed constant.
void addSignedConstant(int64_t Value);
diff --git a/llvm/test/DebugInfo/NVPTX/debug-bool-const-location.ll b/llvm/test/DebugInfo/NVPTX/debug-bool-const-location.ll
new file mode 100644
index 0000000000000..f073d51e7ef99
--- /dev/null
+++ b/llvm/test/DebugInfo/NVPTX/debug-bool-const-location.ll
@@ -0,0 +1,51 @@
+; RUN: llc < %s -asm-verbose -mattr=+ptx76 -O0| FileCheck %s
+; RUN: %if ptxas %{ llc < %s -asm-verbose -mattr=+ptx76 -O0 | %ptxas-verify %}
+
+target triple = "nvptx64-nvidia-cuda"
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-i128:128:128-f32:32:32-f64:64:64-v16:16:16-v32:32:32-v64:64:64-v128:128:128-n16:32:64"
+
+; CHECK: {{.*}}section {{.*}}debug_loc
+; CHECK: .b8 48{{.*}} DW_OP_lit0
+; CHECK: .b8 49{{.*}} DW_OP_lit1
+; CHECK: .b8 144{{.*}} DW_OP_regx
+
+define void @foo(i8 %"arg.arg") !dbg !5
+{
+entry:
+ %".4" = alloca i1
+ %".5" = icmp eq i8 %"arg.arg", 0
+ %arg = alloca i1
+ br i1 %".5", label %"entry.if", label %"entry.else"
+entry.if:
+ store i1 false, i1* %arg
+ call void @"llvm.dbg.value"(metadata i1 false , metadata !9, metadata !10), !dbg !6
+ br label %"entry.endif"
+entry.else:
+ store i1 true, i1* %arg
+ call void @"llvm.dbg.value"(metadata i1 true , metadata !9, metadata !10), !dbg !7
+ br label %"entry.endif"
+entry.endif:
+ %".11" = load i1, i1* %arg
+ store i1 %".11", i1* %".4", !dbg !8
+ call void @"llvm.dbg.value"(metadata i1 %".11" , metadata !9, metadata !10), !dbg !8
+ ret void, !dbg !8
+}
+
+declare void @"llvm.dbg.value"(metadata %".1", metadata %".2", metadata %".3")
+
+!llvm.dbg.cu = !{ !2 }
+!llvm.module.flags = !{ !11, !12 }
+!nvvm.annotations = !{}
+
+!1 = !DIFile(directory: "/source/dir", filename: "test.cu")
+!2 = distinct !DICompileUnit(emissionKind: FullDebug, file: !1, isOptimized: false, language: DW_LANG_C_plus_plus, runtimeVersion: 0)
+!3 = !DIBasicType(encoding: DW_ATE_boolean, name: "bool", size: 8)
+!4 = !DISubroutineType(types: !{null})
+!5 = distinct !DISubprogram(file: !1, isDefinition: true, isLocal: false, isOptimized: false, line: 5, linkageName: "foo", name: "foo", scope: !1, scopeLine: 5, type: !4, unit: !2)
+!6 = !DILocation(column: 1, line: 5, scope: !5)
+!7 = !DILocation(column: 1, line: 7, scope: !5)
+!8 = !DILocation(column: 1, line: 8, scope: !5)
+!9 = !DILocalVariable(arg: 0, file: !1, line: 5, name: "arg", scope: !5, type: !3)
+!10 = !DIExpression()
+!11 = !{ i32 2, !"Dwarf Version", i32 4 }
+!12 = !{ i32 2, !"Debug Info Version", i32 3 }
\ No newline at end of file
diff --git a/llvm/test/DebugInfo/NVPTX/debug-bool-const-value.ll b/llvm/test/DebugInfo/NVPTX/debug-bool-const-value.ll
new file mode 100644
index 0000000000000..002a7a801c746
--- /dev/null
+++ b/llvm/test/DebugInfo/NVPTX/debug-bool-const-value.ll
@@ -0,0 +1,37 @@
+; RUN: llc < %s -asm-verbose -mattr=+ptx76 | FileCheck %s
+; RUN: %if ptxas %{ llc < %s -asm-verbose -mattr=+ptx76 | %ptxas-verify %}
+
+target triple = "nvptx64-nvidia-cuda"
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-i128:128:128-f32:32:32-f64:64:64-v16:16:16-v32:32:32-v64:64:64-v128:128:128-n16:32:64"
+
+; CHECK: {{.*}}section {{.*}}debug_info
+; CHECK: {{.*}}DW_TAG_variable
+; CHECK-NEXT: {{.*}} DW_AT_address_class
+; CHECK-NEXT: .b8 1{{.*}} DW_AT_const_value
+; CHECK-NEXT: {{.*}} DW_AT_name
+
+define void @test() !dbg !5
+{
+entry:
+ %arg = alloca i1
+ store i1 true, i1* %arg, !dbg !6
+ call void @"llvm.dbg.value"(metadata i1 true, metadata !7, metadata !8), !dbg !6
+ ret void, !dbg !6
+}
+
+declare void @"llvm.dbg.value"(metadata %".1", metadata %".2", metadata %".3")
+
+!llvm.dbg.cu = !{ !2 }
+!llvm.module.flags = !{ !9, !10 }
+!nvvm.annotations = !{}
+
+!1 = !DIFile(directory: "/source/dir", filename: "test.cu")
+!2 = distinct !DICompileUnit(emissionKind: FullDebug, file: !1, isOptimized: false, language: DW_LANG_C_plus_plus, runtimeVersion: 0)
+!3 = !DIBasicType(encoding: DW_ATE_boolean, name: "bool", size: 8)
+!4 = !DISubroutineType(types: !{null})
+!5 = distinct !DISubprogram(file: !1, isDefinition: true, isLocal: false, isOptimized: false, line: 5, linkageName: "test", name: "test", scope: !1, scopeLine: 5, type: !4, unit: !2)
+!6 = !DILocation(column: 1, line: 5, scope: !5)
+!7 = !DILocalVariable(arg: 0, file: !1, line: 5, name: "arg", scope: !5, type: !3)
+!8 = !DIExpression()
+!9 = !{ i32 2, !"Dwarf Version", i32 4 }
+!10 = !{ i32 2, !"Debug Info Version", i32 3 }
\ No newline at end of file
|
|
CC: @enferex |
|
CC: @jmorse |
Do you have an example/Github issue of this? Shouldn't the debugger know how to present booleans in a particular language, regardless of which non-zero value was used to indicate truthness? |
Arguable - it's UB to have a bool that isn't 0 or 1 ( So, yeah, I think it is important we describe the value as 0 or 1 if it is 0 or 1, conceptually, at least. |
|
(but yeah, still would be good to see a bug/etc) |
We encountered this issue with |
|
ping |
There was a problem hiding this comment.
Have you considered putting this boolean normalization into addConstantValue? That way, any other place where we emit booleans with out-of-range constants would benefit from this. Some of the overloads don't take a DIType though, so might not be as simple as I imagine?
There was a problem hiding this comment.
I wasn't sure if adding boolean normalization for the APInt or ConstantInt versions made sense, so I only modified the affected call sites.
There was a problem hiding this comment.
I guess what's confusing me is:
- Who is assigning
-1to indicatetrue? - There seem to be two independent changes in this PR:
- emit
DW_OP_litfor boolean constants when computing debug values. This seems very reasonable since it makes the DWARF expression simpler (and works regardless of how the true/false values are represented?) - Don't emit
DW_OP_constu (-1)values for booleans. That also seems reasonable, but what I'm curious about where this-1comes from? And then, if you do this kind of normalization for booleans here, why not any other place whereDW_AT_const_valueis emitted for booleans? You don't strictly have to but I think we can split these two changes into separate PRs
There was a problem hiding this comment.
Who is assigning -1 to indicate true?
llvm-project/llvm/include/llvm/CodeGen/TargetLowering.h
Lines 236 to 241 in 085d0b0
There is a target-dependent
setBooleanContents.
While creating #DBG_VALUE, the true/1 gets signed extednded to 0xffffffffffffffff i.e. -1(Line 736 below)
llvm-project/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
Lines 731 to 749 in 5886a27
And then, if you do this kind of normalization for booleans here, why not any other place where DW_AT_const_value is emitted for booleans?
I checked all the call sites for addConstantValue and found that this is the most relevant one. Alternatively, as you suggested, I could add this normalization to addConstantValue, but we don't have DIType in all overloaded functions.
I think we can split these two changes into separate PRs
Sure. Let me spilt these into two separate PRs- current one for the changes in DwarfCompileUnit.cpp and another one for the changes to emit DW_OP_lit0/1 for booleans.
There was a problem hiding this comment.
emit DW_OP_lit for boolean constants when computing debug values.
Created a separate PR #155539 for these changes
There was a problem hiding this comment.
Hmmm should we address this in instruction selection perhaps then, when we create the debug MachineOperand? I see that SelectionDAG sign extends the constant:
Whereas GlobalISel zero extends it:
So GlobalISel turns the i1 true constant into 1.
My main worry with the normalization you're doing in this patch is that we silently lose information about whether the boolean representation has exhibits the UB that David mentioned. It also feels a bit strange to do the normalization in one place, but not other places where we may emit constants.
@dwblaikie @jmorse This is a bit out of my wheelhouse. Do you have an opinion at which level to do the normalization?
There was a problem hiding this comment.
Yeah, bit outside my wheelhouse too - hopefully @jmorse or other Sony folks who've been working on debug locations lately have some experience to add here.
There was a problem hiding this comment.
It also feels a bit strange to do the normalization in one place, but not other places where we may emit constants.
@Michael137 Adding normalization to addConstantValue will ensure we don't miss anything. We'll also have to change a few call sites that use addConstantValue without DIType. Shall I make this change?
laxmansole
force-pushed
the
user/lsole/boolean_constants_as_literals
branch
from
5810b2f to
f9104e6
Compare
laxmansole
force-pushed
the
user/lsole/boolean_constants_as_literals
branch
from
f9104e6 to
a3fbfa6
Compare
laxmansole
force-pushed
the
user/lsole/boolean_constants_as_literals
branch
from
a3fbfa6 to
859a086
Compare
|
Ping |
There was a problem hiding this comment.
Hmmm should we address this in instruction selection perhaps then, when we create the debug MachineOperand? I see that SelectionDAG sign extends the constant:
Whereas GlobalISel zero extends it:
So GlobalISel turns the i1 true constant into 1.
My main worry with the normalization you're doing in this patch is that we silently lose information about whether the boolean representation has exhibits the UB that David mentioned. It also feels a bit strange to do the normalization in one place, but not other places where we may emit constants.
@dwblaikie @jmorse This is a bit out of my wheelhouse. Do you have an opinion at which level to do the normalization?
laxmansole
changed the title
|
Breaking out from the inline thread into the main one -- sorry for the silence, I've been sunk by other stuff over the holidays, I'm leaning towards agreeing with @Michael137, while this patch is good I think the problem is better solved in instruction-selection. Looking at the history of the highlighted code, the sign extension appeared in 563fe3c (svn r99469), where there isn't any indication that sign-extending to all-ones is the intended behaviour of the patch. Stepping back further, a lot of this variation comes from the fact that type-system for variable locations is non-existent (cc @slinder1 ). Booleans will take on the IR type of whatever the bool is stored-as or computed by: in this godbolt example https://godbolt.org/z/PWodh7qo1 in the "beans" function output, the inlined boolean variable "bar" is described by constants with both i8 and i1 type (see metadata node !22). This is because the first dbg.value is a constant promoted off the stack where it's stored as an i8, while the second dbg.value comes from an icmp instruction. I reckon with a bit more work we could create an example where the Boolean True value is represented as both 1 and -1 in the same function after isel, which is IMO undesirable. I don't fully understand the context about NVPTX describing True with -1: I assume it's alright to always describe True with positive 1 in DWARF even if negative 1 is an option, and debuggers will "just know" how to fixup the two. IMHO (and there's a reasonable chance I'm wrong) the appearance of |
Sign extension is required here, otherwise we'll generate an incorrect result for the following case, where the constant value is negative - Constant boolean value We can check the at the place of llvm-project/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp Lines 110 to 113 in 3dc314b and at the place of llvm-project/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp Lines 734 to 736 in a80c393 |
|
That makes sense (to be selective about the bitwidths). A slight downside is that then this canonicalization happens in several places instead of one, however IMO that's worth it for test consistency. (I wondered whether we could be more specific and check whether the variable encoding type is DW_ATE_boolean... however we might then have difficulty with variadic variable locations, where the variable type is different from the operands used to compute the variable value. So that's probably a no-go). |
llvmbot
added
llvm:globalisel
llvm:SelectionDAG
There was a problem hiding this comment.
Why not just handle this when emitting the debug info? Why does this need a special case in mir
There was a problem hiding this comment.
TLDR; this should not be special cased. CodeGen imm bool fields are firmly -1
There was a problem hiding this comment.
Could you elaborate please -- is there an existing convention to that effect for instructions, and does that necessarily extend to debug instructions too? As illustrated in the main thread LLVM inconsistently types variable locations for constant Booleans during optimisation, which creates the need for special-casing. This canonicalization both solves a DWARF-output-flaw, but will also improve variable location coverage by avoiding inconsistent constant-values (variable locations can be spuriously dropped as a result).
(We can cannonicalise from Booleans up to integer-one as in this patch, but it's a lot more difficult to ensure the validity of converting non-false-non-i1 LLVM-IR values down to Boolean -1).
There was a problem hiding this comment.
Pretty much everywhere in codegen sign extends constants. i.e., MachineOperand's MO_Immediate is int64_t not uint64_t.
There was a problem hiding this comment.
Sign-extension is effective for handling both signed and unsigned values. However, in the case of 1-bit values, it causes true to become -1, which isn't the desired result. Special case ensures that true remains 1.
There was a problem hiding this comment.
@arsenm I agree it's best to keep the convention for codegen; however I'd argue that this behaviour in this function isn't codegen, it's generating a debug-instruction that will (should) be ignored by every pieces of code that does codegen. In an ideal world we would be able to peel these two concerns (operand consistency for codegen; normalisation for debug-info) apart, and we're working towards that [0], but it's currently imperfect and this specialisation for debug-info will improve things in the meantime.
[0] https://discourse.llvm.org/t/psa-ir-output-changing-from-debug-intrinsics-to-debug-records/79578/11
There was a problem hiding this comment.
But how is this easier than just fixing up whatever emits the final object file / asm printing?
There was a problem hiding this comment.
This whole discussion started on a version of the patch where we tried to do this in debug-info. It wasn't clear which parts of debug-info emission should be responsible for this. And we don't want to fix-up legitimately corrupted booleans. We'd want this those cases to show up in the debugger. Instruction selection being the one to adjust it for debug ops seemed like the more appropriate place to do this at (though I'm by no means an instruction selection expert; just from stepping through this scenario, this is where the -1 booleans were being propagated to debug-info).
There was a problem hiding this comment.
IMO isel is the right place to put this today (ideally in a year this would all be place in a non-instruction format). It can't be earlier because LLVM-IR transforms will continue to mutate the type of operands to dbg.values. If it's later than isel, it'd need to be in three places (LiveDebugValues, location-list builder, DWARF emitter) so that identical-booleans can be recognised as such and have location-ranges merged (or at least, not conflict and become "optimised out"), which is what we risk with a de-normalised boolean-true. Plus: isel is the natural place to normalise when we reach a non-instruction debug-info format in MIR.
Again, I'm not saying breaking convention here isn't a bad thing, IMO it's the least worst of several normalisation options.
laxmansole
force-pushed
the
user/lsole/boolean_constants_as_literals
branch
from
fca6d44 to
2896a07
Compare
| if (CI->getBitWidth() > 64) | ||
| MIB.addCImm(CI); | ||
| else | ||
| else if (CI->getBitWidth() == 1) |
There was a problem hiding this comment.
Why are we modifying GlobalISel? I think it was working correctly there already right?
There was a problem hiding this comment.
Immediate values need to be sign-extended for correctness. Consider a case where the immediate operand is a negative value.
An exception to this would be 1-bit values, as they don't have a notion of sign, and sign-extending them changes their value like 1 becomes -1.
|
Ping |
1 similar comment
|
Ping |
|
Ping |
|
Ping @arsenm |
jmorse
left a comment
jmorse
left a comment
There was a problem hiding this comment.
I had a chat with @arsenm at the conference about this -- the summary of which is "yeah it sucks to have debug instructions", but no strong resistance to this particular patch. I'm happy to assert that Matts, if not happy, then broadly OK, with this patch landing.
(Someday soon we'll get rid of debug instructions post codegen, but it is not this day).
Awesome, thanks for following up on this! |
@jmorse Is there a description of the imagined design for this somewhere? This is first I've heard of such a plan, so I am curious to hear more. (It feels like it would be too off-topic to spell out the details in this PR, but I'm curious if there's an RFC or similar you can link me to.) |
There's a bit of an outline here and a link to some extremely rough code: https://discourse.llvm.org/t/psa-ir-output-changing-from-debug-intrinsics-to-debug-records/79578/11 |
Thanks all for reviewing and sharing your perspectives — this was a great discussion! |
laxmansole
force-pushed
the
user/lsole/boolean_constants_as_literals
branch
from
2896a07 to
32d4b26
Compare
laxmansole
force-pushed
the
user/lsole/boolean_constants_as_literals
branch
from
32d4b26 to
0968b1f
Compare
|
@laxmansole Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
8 participants
Previously, sign-extending a 1-bit boolean operand in
#DBG_VALUEwould converttrueto -1 (i.e., 0xffffffffffffffff). However, DWARF treats booleans as unsigned values, so this resulted in the attributeDW_AT_const_value(0xffffffffffffffff)being emitted. As a result, the debugger would display the value as255instead oftrue.This change modifies the behavior to use zero-extension for 1-bit values instead, ensuring that
trueis represented as 1. Consequently, the DWARF attribute emitted is nowDW_AT_const_value(1), which allows the debugger to correctly display the boolean astrue.