Minor optimizations to DW_OP_LLVM_extract_bits_*

[tromey]

I noticed a couple more small optimization opportunities when generating DWARF expressions from the internal
DW_OP_LLVM_extract_bits_* operations:

• DW_OP_deref can be used, rather than DW_OP_deref_size, when the deref size is the word size.

• If the bit offset is 0 and an unsigned extraction is desired, then sometimes the shifting can be skipped entirely, or replaced with DW_OP_and.

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Tom Tromey (tromey)

Changes

I noticed a couple more small optimization opportunities when generating DWARF expressions from the internal
DW_OP_LLVM_extract_bits_* operations:

• DW_OP_deref can be used, rather than DW_OP_deref_size, when the deref size is the word size.

• If the bit offset is 0 and an unsigned extraction is desired, then sometimes the shifting can be skipped entirely, or replaced with DW_OP_and.

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp (+41-19)
• (modified) llvm/test/DebugInfo/X86/DW_OP_LLVM_extract_bits.ll (+3-3)

diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
index f0f086184656a..c7d45897c403b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
@@ -566,32 +566,54 @@ bool DwarfExpression::addExpression(
     case dwarf::DW_OP_LLVM_extract_bits_zext: {
       unsigned SizeInBits = Op->getArg(1);
       unsigned BitOffset = Op->getArg(0);
+      unsigned DerefSize = 0;
+      //  Operations are done in the DWARF "generic type" whose size
+      // is the size of a pointer.
+      unsigned PtrSizeInBytes = CU.getAsmPrinter()->MAI->getCodePointerSize();
 
       // If we have a memory location then dereference to get the value, though
       // we have to make sure we don't dereference any bytes past the end of the
       // object.
       if (isMemoryLocation()) {
-        emitOp(dwarf::DW_OP_deref_size);
-        emitUnsigned(alignTo(BitOffset + SizeInBits, 8) / 8);
+        DerefSize = alignTo(BitOffset + SizeInBits, 8) / 8;
+        if (DerefSize == PtrSizeInBytes) {
+          emitOp(dwarf::DW_OP_deref);
+        } else {
+          emitOp(dwarf::DW_OP_deref_size);
+          emitUnsigned(DerefSize);
+        }
       }
 
-      // Extract the bits by a shift left (to shift out the bits after what we
-      // want to extract) followed by shift right (to shift the bits to position
-      // 0 and also sign/zero extend). These operations are done in the DWARF
-      // "generic type" whose size is the size of a pointer.
-      unsigned PtrSizeInBytes = CU.getAsmPrinter()->MAI->getCodePointerSize();
-      unsigned LeftShift = PtrSizeInBytes * 8 - (SizeInBits + BitOffset);
-      unsigned RightShift = LeftShift + BitOffset;
-      if (LeftShift) {
-        emitOp(dwarf::DW_OP_constu);
-        emitUnsigned(LeftShift);
-        emitOp(dwarf::DW_OP_shl);
-      }
-      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);
+      // If a dereference was emitted for an unsigned value, and
+      // there's no bit offset, then a bit of optimization is
+      // possible.
+      if (OpNum == dwarf::DW_OP_LLVM_extract_bits_zext && BitOffset == 0) {
+        if (8 * DerefSize == SizeInBits) {
+          // The correct value is already on the stack.
+        } else {
+          // No need to shift, we can just mask off the desired bits.
+          emitOp(dwarf::DW_OP_constu);
+          emitUnsigned((1u << SizeInBits) - 1);
+          emitOp(dwarf::DW_OP_and);
+        }
+      } else {
+        // Extract the bits by a shift left (to shift out the bits after what we
+        // want to extract) followed by shift right (to shift the bits to
+        // position 0 and also sign/zero extend).
+        unsigned LeftShift = PtrSizeInBytes * 8 - (SizeInBits + BitOffset);
+        unsigned RightShift = LeftShift + BitOffset;
+        if (LeftShift) {
+          emitOp(dwarf::DW_OP_constu);
+          emitUnsigned(LeftShift);
+          emitOp(dwarf::DW_OP_shl);
+        }
+        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
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 5928127866f7e..b79c84d532786 100644
--- a/llvm/test/DebugInfo/X86/DW_OP_LLVM_extract_bits.ll
+++ b/llvm/test/DebugInfo/X86/DW_OP_LLVM_extract_bits.ll
@@ -8,7 +8,7 @@
 ; CHECK-LABEL: DW_TAG_subprogram
 ; CHECK: DW_AT_name ("test1")
 ; CHECK: DW_TAG_variable
-; CHECK: DW_AT_location (DW_OP_fbreg -1, DW_OP_deref_size 0x1, DW_OP_constu 0x3d, DW_OP_shl, DW_OP_constu 0x3d, DW_OP_shr, DW_OP_stack_value)
+; CHECK: DW_AT_location (DW_OP_fbreg -1, DW_OP_deref_size 0x1, DW_OP_constu 0x7, DW_OP_and, DW_OP_stack_value)
 ; CHECK: DW_AT_name ("x")
 ; CHECK: DW_TAG_variable
 ; CHECK: DW_AT_location (DW_OP_fbreg -1, DW_OP_deref_size 0x1, DW_OP_constu 0x39, DW_OP_shl, DW_OP_constu 0x3c, DW_OP_shra, DW_OP_stack_value)
@@ -25,7 +25,7 @@ entry:
 ; CHECK-LABEL: DW_TAG_subprogram
 ; CHECK: DW_AT_name ("test2")
 ; CHECK: DW_TAG_variable
-; CHECK: DW_AT_location (DW_OP_breg0 {{R[^+]+}}+0, DW_OP_constu 0xff, DW_OP_and, DW_OP_constu 0x3d, DW_OP_shl, DW_OP_constu 0x3d, DW_OP_shr, DW_OP_stack_value)
+; CHECK: DW_AT_location (DW_OP_breg0 {{R[^+]+}}+0, DW_OP_constu 0xff, DW_OP_and, DW_OP_constu 0x7, DW_OP_and, DW_OP_stack_value)
 ; CHECK: DW_AT_name ("x")
 ; CHECK: DW_TAG_variable
 ; CHECK: DW_AT_location (DW_OP_breg0 {{R[^+]+}}+0, DW_OP_constu 0xff, DW_OP_and, DW_OP_constu 0x39, DW_OP_shl, DW_OP_constu 0x3c, DW_OP_shra, DW_OP_stack_value)
@@ -67,7 +67,7 @@ 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_location (DW_OP_fbreg -4, DW_OP_deref, DW_OP_stack_value)
 ; CHECK: DW_AT_name ("q")
 
 define i32 @test4() !dbg !28 {

[jryans]

Thanks, this looks good to me! 😄

I'll wait a few days for others to chime in, then I'll plan to merge this assuming the CI checks pass.

[Michael137]

Might be totally off but didnt @rastogishubham and @adrian-prantl add a Dwarf expression optimizer not too long ago? Could we build this into that?

[jryans]

Might be totally off but didnt @rastogishubham and @adrian-prantl add a Dwarf expression optimizer not too long ago? Could we build this into that?

For others who may be wondering, this DWARF expression optimiser is found in llvm/lib/IR/DIExpressionOptimizer.cpp. After looking over that optimiser, it seems like it focuses on simplifying operations without inspecting operand details like size in bits etc.

The changes in this PR rely on checking such operand bit sizes and then performing a "final refinement" of the emitted expressions accordingly.

I imagine DIExpressionOptimizer could be extended to consider operand details as well, but after looking over the code, it doesn't seem obvious to me that its the best place to do so at the moment. In any case, it would be to good hear an opinion from one of the people involved with that optimiser to double-check my thinking here. 🙂

[rastogishubham]

@jryans I agree, this PR doesn't seem like it would fit with the expression optimizer. It is separate, and looks good to me to land

[jryans]

I'll be travelling the first few days next week... Assuming no other feedback, I'll plan to merge this Thursday (23 Oct).

[OCHyams]

+1 SGTM

[jryans]

Had a bit of time today, and we've got multiple approvals, so I'll merge now. Thanks all! 😄