Do not emit right shift by 0 in DWARF expressions

[tromey]

DW_OP_LLVM_extract_bits_zext and DW_OP_LLVM_extract_bits_sext can end up emitting "DW_OP_constu 0; DW_OP_shr" when given certain arguments. However, a shift by zero is not useful, and so it can be omitted.

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Tom Tromey (tromey)

Changes

DW_OP_LLVM_extract_bits_zext and DW_OP_LLVM_extract_bits_sext can end up emitting "DW_OP_constu 0; DW_OP_shr" when given certain arguments. However, a shift by zero is not useful, and so it can be omitted.

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp (+6-4)
• (modified) llvm/test/DebugInfo/X86/DW_OP_LLVM_extract_bits.ll (+5)

diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
index bc0bb349be249..f0f086184656a 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
@@ -587,10 +587,12 @@ bool DwarfExpression::addExpression(
         emitUnsigned(LeftShift);
         emitOp(dwarf::DW_OP_shl);
       }
-      emitOp(dwarf::DW_OP_constu);
-      emitUnsigned(RightShift);
-      emitOp(OpNum == dwarf::DW_OP_LLVM_extract_bits_sext ? dwarf::DW_OP_shra
-                                                          : dwarf::DW_OP_shr);
+      if (RightShift) {
+        emitOp(dwarf::DW_OP_constu);
+        emitUnsigned(RightShift);
+        emitOp(OpNum == dwarf::DW_OP_LLVM_extract_bits_sext ? dwarf::DW_OP_shra
+                                                            : dwarf::DW_OP_shr);
+      }
 
       // The value is now at the top of the stack, so set the location to
       // implicit so that we get a stack_value at the end.
diff --git a/llvm/test/DebugInfo/X86/DW_OP_LLVM_extract_bits.ll b/llvm/test/DebugInfo/X86/DW_OP_LLVM_extract_bits.ll
index 8342d42f6a292..5928127866f7e 100644
--- a/llvm/test/DebugInfo/X86/DW_OP_LLVM_extract_bits.ll
+++ b/llvm/test/DebugInfo/X86/DW_OP_LLVM_extract_bits.ll
@@ -67,6 +67,8 @@ entry:
 ; CHECK: DW_TAG_variable
 ; CHECK: DW_AT_location (DW_OP_fbreg -4, DW_OP_plus_uconst 0x3, DW_OP_deref_size 0x1, DW_OP_constu 0x38, DW_OP_shl, DW_OP_constu 0x39, DW_OP_shr, DW_OP_stack_value)
 ; CHECK: DW_AT_name ("z")
+; CHECK: DW_AT_location (DW_OP_fbreg -4, DW_OP_deref_size 0x8, DW_OP_stack_value)
+; CHECK: DW_AT_name ("q")
 
 define i32 @test4() !dbg !28 {
 entry:
@@ -74,6 +76,7 @@ entry:
   tail call void @llvm.dbg.declare(metadata ptr %0, metadata !29, metadata !DIExpression(DW_OP_LLVM_extract_bits_zext, 31, 1)), !dbg !30
   tail call void @llvm.dbg.declare(metadata ptr %0, metadata !31, metadata !DIExpression(DW_OP_LLVM_extract_bits_sext, 1, 31)), !dbg !30
   tail call void @llvm.dbg.declare(metadata ptr %0, metadata !32, metadata !DIExpression(DW_OP_plus_uconst, 3, DW_OP_LLVM_extract_bits_zext, 1, 7)), !dbg !30
+  tail call void @llvm.dbg.declare(metadata ptr %0, metadata !33, metadata !DIExpression(DW_OP_LLVM_extract_bits_zext, 0, 64)), !dbg !30
   ret i32 0, !dbg !30
 }
 
@@ -116,3 +119,5 @@ declare void @llvm.dbg.value(metadata, metadata, metadata)
 !30 = !DILocation(line: 0, scope: !28)
 !31 = !DILocalVariable(name: "y", scope: !28, file: !3, type: !19)
 !32 = !DILocalVariable(name: "z", scope: !28, file: !3, type: !9)
+!33 = !DILocalVariable(name: "q", scope: !28, file: !3, type: !34)
+!34 = !DIBasicType(name: "uint64_t", size: 64, encoding: DW_ATE_unsigned)

[OCHyams]

LGTM and makes sense - curious how you found this and/or if you have a motivating example?

[tromey]

LGTM and makes sense - curious how you found this and/or if you have a motivating example?

I found it while working on gnat-llvm. Ada can generate somewhat complicated location expressions for field offsets, and I've been having the gnat-llvm DWARF expression generator always generate DW_OP_LLVM_extract_bits_* when referencing a discriminant, because this already handles any needed masking and sign extension. So I stumbled across this when debugging a test case where the discriminant is a full word.

I don't have permissions to check this in, so could you do that? Thank you.

[tromey]

FWIW I found another case here that can be improved, I'll send a new patch in a bit.