[Utils] Salvage the debug info of DCE'ed 'xor' instructions

[vsk]

@djtodoro, I was hoping to rebase and land this after D43163 (taking care of test updates as needed). Wdyt?

[aprantl]

I wonder if this actually happens in the wild, but it doesn't hurt.

[ibadawi]

LGTM

[vsk]

It looks like this is able to salvage actual variables in the wild:

Assertion failed: (false && "hi! salvaged a xor!"), function salvageDebugInfo
...
Running pass 'CallGraph Pass Manager' on module '/Users/vsk/src/llvm.org-debugify/llvm/lib/Support/Regex.cpp'.

Assertion failed: (false && "hi! salvaged a xor!"), function salvageDebugInfo
...
Running pass 'CallGraph Pass Manager' on module '/Users/vsk/src/llvm.org-debugify/llvm/lib/Support/APInt.cpp'.

[vsk]

Here are some better numbers from a stage2 build of clang:

12273 SALVAGE: ADD
14184 SALVAGE: MISSED
1064 SALVAGE: OR
  14 SALVAGE: SUB
 259 SALVAGE: XOR

The "MISSED" case refers to binary operators we simply haven't taught salvageDebugInfo() about.

[aprantl]

Thanks! Might be worth also measuring the size of .debug_loc as we are doing this. We may need to start compressing locations by using DWARF procedures for common patterns (DW_OP_call).

[vsk]

I extended this patch to salvage debug values from mul/sdiv/srem, shl/lhsr/ashr, and sub instructions (in addition to xor instructions). Next, I prepared dsyms for stage2 builds of clang (-O3 -g). Here are the results:

pre-patch
---------

Segment __DWARF: 814804992
        Section __debug_line: 36805639     (35.10 MB)
        Section __debug_pubnames: 8291810
        Section __debug_pubtypes: 62606366
        Section __debug_ranges: 43286544
        Section __debug_loc: 122491744     (116.81 MB)
        Section __debug_aranges: 986672
        Section __debug_info: 379630571    (362.04 MB)
        Section __debug_abbrev: 10685
        Section __debug_str: 106914677
        Section __apple_names: 35657500
        Section __apple_namespac: 50616
        Section __apple_types: 18069091
        Section __apple_objc: 36
        total 814801951

{"version":"1","file":"pre-patches-clang.dSYM/Contents/Resources/DWARF/clang-7.0","format":"Mach-O 64-bit x86-64","source functions":309766,"inlined functions":3351149,"unique source variables":1981705,"source variables":403779101,"variables with location":4127221,"scope bytes total":522119469,"scope bytes covered":305284275}

post-patch
----------

Salvaging dbg.values from:
mul/sdiv/srem, shl/lhsr/ashr, sub, xor

Segment __DWARF: 814891008
        Section __debug_line: 36806878     (35.10 MB, +0%)
        Section __debug_pubnames: 8291810
        Section __debug_pubtypes: 62606366
        Section __debug_ranges: 43287504
        Section __debug_loc: 122557034     (116.88 MB, +0.06%)
        Section __debug_aranges: 986672
        Section __debug_info: 379647347    (362.06 MB, +0%)
        Section __debug_abbrev: 10685
        Section __debug_str: 106914677
        Section __apple_names: 35658692
        Section __apple_namespac: 50616
        Section __apple_types: 18069091
        Section __apple_objc: 36
        total 814887408

{"version":"1","file":"post-patches-clang.dSYM/Contents/Resources/DWARF/clang-7.0","format":"Mach-O 64-bit x86-64","source functions":309766,"inlined functions":3351298,"unique source variables":1982115,"source variables":403824889,"variables with location":4128581,"scope bytes total":522201413,"scope bytes covered":305416207}

There is a modest increase of the debug_loc section, which indicates that users will notice a small increase in the number of available debug values, without seeing too much binary size bloat.

I also included statistics from dwarfdump: post-patch, we have 410 additional unique source variables available.

[aprantl]

Those numbers look good!

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

case dwarf::DW_OP_plus_uconst:
       assert(LocationKind != Register);
       emitOp(dwarf::DW_OP_plus_uconst);
       emitUnsigned(Op->getArg(0));
       break;
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
     case dwarf::DW_OP_mul:
     case dwarf::DW_OP_or:
+    case dwarf::DW_OP_xor:
       emitOp(Op->getOp());
       break;
     case dwarf::DW_OP_deref:
       assert(LocationKind != Register);
       if (LocationKind != Memory && isMemoryLocation(ExprCursor))
         // Turning this into a memory location description makes the deref
         // implicit.
         LocationKind = Memory;

lib/IR/DebugInfoMetadata.cpp

case dwarf::DW_OP_swap: {
       break;
     }
     case dwarf::DW_OP_constu:
     case dwarf::DW_OP_plus_uconst:
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
     case dwarf::DW_OP_mul:
     case dwarf::DW_OP_or:
+    case dwarf::DW_OP_xor:
     case dwarf::DW_OP_deref:
     case dwarf::DW_OP_xderef:
       break;
     }
   }
   return true;
 }
 

lib/Transforms/Utils/Local.cpp

     // Rewrite a constant GEP into a DIExpression.  Since we are performing
     // arithmetic to compute the variable's *value* in the DIExpression, we
     // need to mark the expression with a DW_OP_stack_value.
     APInt Offset(BitWidth, 0);
     if (GEP->accumulateConstantOffset(M.getDataLayout(), Offset))
       for (auto *DII : DbgUsers)
         applyOffset(DII, Offset.getSExtValue());
   } else if (auto *BI = dyn_cast<BinaryOperator>(&I)) {
-    if (BI->getOpcode() == Instruction::Add ||
-        BI->getOpcode() == Instruction::Or)
-      if (auto *ConstInt = dyn_cast<ConstantInt>(I.getOperand(1)))
-        if (ConstInt->getBitWidth() <= 64) {
-          uint64_t Val = ConstInt->getSExtValue();
-          for (auto *DII : DbgUsers)
-            if (BI->getOpcode() == Instruction::Add)
-              applyOffset(DII, Val);
-            else if (BI->getOpcode() == Instruction::Or)
-              applyOps(DII, {dwarf::DW_OP_constu, Val, dwarf::DW_OP_or});
-        }
+    auto *ConstInt = dyn_cast<ConstantInt>(I.getOperand(1));
+    if (!ConstInt || ConstInt->getBitWidth() > 64)
+      return;
+
+    for (auto *DII : DbgUsers) {
+      switch (BI->getOpcode()) {
+      case Instruction::Add:
+        applyOffset(DII, Val);
+        break;
+      case Instruction::Or:
+        applyOps(DII, {dwarf::DW_OP_constu, Val, dwarf::DW_OP_or});
+        break;
+      case Instruction::Xor:
+        applyOps(DII, {dwarf::DW_OP_constu, Val, dwarf::DW_OP_xor});
+        break;
+      default:
+        // TODO: Salvage constants from each kind of binop we know about.
+        continue;
+      }
+    }
   } else if (isa<LoadInst>(&I)) {
     MetadataAsValue *AddrMD = wrapMD(I.getOperand(0));
     for (auto *DII : DbgUsers) {
       // Rewrite the load into DW_OP_deref.
       auto *DIExpr = DII->getExpression();
       DIExpr = DIExpression::prepend(DIExpr, DIExpression::WithDeref);
       DII->setOperand(0, AddrMD);
       DII->setOperand(2, MetadataAsValue::get(I.getContext(), DIExpr));

test/Transforms/InstCombine/debuginfo-variables.ll

 
 define void @test_or(i64 %A) {
 ; CHECK-LABEL: @test_or(
 ; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %A, metadata !17, metadata !DIExpression(DW_OP_constu, 256, DW_OP_or, DW_OP_stack_value)), !dbg !18
   %1 = or i64 %A, 256
   ret void
 }
 
+define void @test_xor(i32 %A) {
+; CHECK-LABEL: @test_xor(
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i32 %A, metadata !22, metadata !DIExpression(DW_OP_constu, 1, DW_OP_xor, DW_OP_stack_value)), !dbg !23
+  %xor = xor i32 %A, 1
+  ret void
+}
+
 ; CHECK: !8 = !DILocalVariable(name: "1", scope: !5, file: !1, line: 1, type: !9)
 ; CHECK: !10 = !DILocalVariable(name: "2", scope: !5, file: !1, line: 2, type: !11)
 ; CHECK: !12 = !DILocation(line: 2, column: 1, scope: !5)
 ; CHECK: !13 = !DILocation(line: 1, column: 1, scope: !5)
 
 ; CHECK: !17 = !DILocalVariable(name: "3", scope: !15, file: !1, line: 4, type: !11)
 ; CHECK: !18 = !DILocation(line: 4, column: 1, scope: !15)
+
+; CHECK: !22 = !DILocalVariable(name: "4", scope: !20, file: !1, line: 6, type: !9)
+; CHECK: !23 = !DILocation(line: 6, column: 1, scope: !20)