Improve virtual register handling when computing debug information

[ddcc]

Summary

Some backends, like WebAssembly, use virtual registers instead of physical registers. This crashes the DbgValueHistoryCalculator pass, which assumes that all registers are physical. Instead, skip virtual registers when iterating aliases, and assume that they are clobbered.

[dschuff]

I think this surely isn't the final solution for debug info on virtual targets like wasm (that will probably require some more design work in the medium term), but not crashing is certainly a good start.
So, I think this code is fine. But the first part of the commit description isn't quite right, because it doesn't actually ignore virtual registers; it does track the clobbers (it just knows they don't have aliases and doesn't try to put them into the set of changing registers).

[ddcc]

Right, I've fixed the title.

[dschuff]

LGTM for a start; we'll wait a bit and see if there are other opinions before landing.

[aprantl]

Looks like this could really benefit from a (MIR?) testcase.

[dschuff]

Yeah, it would have to be a MIR testcase; I wasn't sure if those were ready for prime time (and in particular we don't have any in the wasm backend). I can look into that; FWIW the test cases in D21808 do cover this (although they are obviously less well-targeted since they are traditional ll->asm codegen test cases.

[dschuff]

OK I looked a little more wrt tests. The problem with MIR in this case is that DbgValueHistoryCalculator isn't a pass, it's just part of the AsmPrinter pass, which isn't really set up for MIR testing (e.g. for use with llc -run-pass etc). Maybe we should just add a regular wasm codegen test? It's not quite ideal since this is target-independent code, but it would cover this, and it could still be pretty simple.

[aprantl]

What about a test that uses MIR as *input* (so isn't affected by future codegen changes) and then checks DWARF output?

[ddcc]

Add test

[ddcc]

I added a regular codegen test. I tried the MIR route, but ran into multiple bugs, including broken branch probability parsing, and no support for target passes.

[dschuff]

Ouch, I think https://llvm.org/bugs/show_bug.cgi?id=24261 is probably a showstopper for MIR tests in wasm. Basically all the interesting things we'd want to test are target passes, and some of those passes need to be run in order to do anything interesting at all. I'm guessing the problem for this particular test is that you'd need to start after WebAssemblyRegNumbering in order to feed this into the asmprinter?

[dschuff]

Also, looking at the branch probability parsing code, it looks like https://llvm.org/bugs/show_bug.cgi?id=28751 has been broken since December D:

[ddcc]

Yes, without running the target passes for WebAssembly, llc will just crash later due to the virtual registers. Is this patch OK to land as is with the regular test?

[dschuff]

I think we should land this with a regular test, because that's better than no test. We should also fix the llc pass runner because I would really like to have some more targeted tests for the wasm backend passes, but I don't want to block you on that.

[ddcc]

Is this OK to merge?

[aprantl]

I'm planning to simplify DbgValueHistoryCalculator in the near future so I'll need all the test coverage I can get :-)

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

249–250

I can comment out this condition and the test still passes.

[ddcc]

Update test

[aprantl]

Thanks!

[dschuff]

Thanks for pushing this through to the end!

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

 
 // \brief If @MI is a DBG_VALUE with debug value described by a
 // defined register, returns the number of this register.
 // In the other case, returns 0.
 static unsigned isDescribedByReg(const MachineInstr &MI) {
   assert(MI.isDebugValue());
   assert(MI.getNumOperands() == 4);
   // If location of variable is described using a register (directly or
-  // indirecltly), this register is always a first operand.
+  // indirectly), this register is always a first operand.
   return MI.getOperand(0).isReg() ? MI.getOperand(0).getReg() : 0;
 }
 
 void DbgValueHistoryMap::startInstrRange(InlinedVariable Var,
                                          const MachineInstr &MI) {
   // Instruction range should start with a DBG_VALUE instruction for the
   // variable.
   assert(MI.isDebugValue() && "not a DBG_VALUE");
for (const auto &MI : MBB) {
       if (&MI == FirstEpilogueInst)
         break;
       if (MI.getFlag(MachineInstr::FrameSetup))
         continue;
 
       // Look for register defs and register masks. Register masks are
       // typically on calls and they clobber everything not in the mask.
       for (const MachineOperand &MO : MI.operands()) {
-        if (MO.isReg() && MO.isDef() && MO.getReg()) {
+        // Skip virtual registers since they are handled by the parent.
+        if (MO.isReg() && MO.isDef() && MO.getReg() &&
+            !TRI->isVirtualRegister(MO.getReg())) {
           for (MCRegAliasIterator AI(MO.getReg(), TRI, true); AI.isValid();
                ++AI)
             Regs.set(*AI);
         } else if (MO.isRegMask()) {
           Regs.setBitsNotInMask(MO.getRegMask());
         }
       }
     }
void llvm::calculateDbgValueHistory(const MachineFunction *MF,
   RegDescribedVarsMap RegVars;
   for (const auto &MBB : *MF) {
     for (const auto &MI : MBB) {
       if (!MI.isDebugValue()) {
         // Not a DBG_VALUE instruction. It may clobber registers which describe
         // some variables.
         for (const MachineOperand &MO : MI.operands()) {
           if (MO.isReg() && MO.isDef() && MO.getReg()) {
+            // If this is a virtual register, only clobber it since it doesn't
+            // have aliases.
+            if (TRI->isVirtualRegister(MO.getReg()))
+              clobberRegisterUses(RegVars, MO.getReg(), Result, MI);
             // If this is a register def operand, it may end a debug value
             // range.
+            else {
               for (MCRegAliasIterator AI(MO.getReg(), TRI, true); AI.isValid();
                    ++AI)
                 if (ChangingRegs.test(*AI))
                   clobberRegisterUses(RegVars, *AI, Result, MI);
+            }
           } else if (MO.isRegMask()) {
             // If this is a register mask operand, clobber all debug values in
             // non-CSRs.
             for (int I = ChangingRegs.find_first(); I != -1;
                  I = ChangingRegs.find_next(I)) {
               // Don't consider SP to be clobbered by register masks.
               if (unsigned(I) != SP && TRI->isPhysicalRegister(I) &&
                   MO.clobbersPhysReg(I)) {
for (const auto &MBB : *MF) {
     }
 
     // Make sure locations for register-described variables are valid only
     // until the end of the basic block (unless it's the last basic block, in
     // which case let their liveness run off to the end of the function).
     if (!MBB.empty() && &MBB != &MF->back()) {
       for (auto I = RegVars.begin(), E = RegVars.end(); I != E;) {
         auto CurElem = I++; // CurElem can be erased below.
-        if (ChangingRegs.test(CurElem->first))
+        if (TRI->isVirtualRegister(CurElem->first) ||
+            ChangingRegs.test(CurElem->first))
           clobberRegisterUses(RegVars, CurElem, Result, MBB.back());
       }
     }
   }
 }

test/CodeGen/WebAssembly/dbgvalue.ll

+; RUN: llc < %s -O0 -verify-machineinstrs -mtriple=wasm32-unknown-unknown | FileCheck %s
+; CHECK: BB#0
+; CHECK: #DEBUG_VALUE: usage:self <- %vreg4
+; CHECK: BB#1
+; CHECK: DW_TAG_variable
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+%0 = type opaque
+
+@key = external local_unnamed_addr global [15 x i8], align 1
+@.str = external unnamed_addr constant [33 x i8], align 1
+
+; Function Attrs: nounwind
+define internal i32 @0(i8*) local_unnamed_addr !dbg !14 !type !22 {
+  tail call void @llvm.dbg.value(metadata i8* %0, i64 0, metadata !21, metadata !23), !dbg !24
+  %2 = tail call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([33 x i8], [33 x i8]* @.str, i32 0, i32 0), i8* %0), !dbg !25
+  br i1 1, label %a, label %b
+a:
+  %3 = add i32 %2, %2
+  br label %c
+
+b:
+  %4 = sub i32 %2, %2
+  br label %c
+
+c:
+  %5 = phi i32 [ %3, %a ], [ %4, %b ]
+  %6 = add i32 ptrtoint (i32 (i8*)* @0 to i32), %5
+  ret i32 %6, !dbg !26
+}
+
+; Function Attrs: nounwind
+declare i32 @printf(i8* nocapture readonly, ...) local_unnamed_addr
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!11, !12}
+!llvm.ident = !{!13}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.9.0 (trunk 273884) (llvm/trunk 273897)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3)
+!1 = !DIFile(filename: "crash.c", directory: "wasm/tests")
+!2 = !{}
+!3 = !{!4}
+!4 = distinct !DIGlobalVariable(name: "key", scope: !0, file: !1, line: 7, type: !5, isLocal: false, isDefinition: true, variable: [15 x i8]* @key)
+!5 = !DICompositeType(tag: DW_TAG_array_type, baseType: !6, size: 120, align: 8, elements: !9)
+!6 = !DIDerivedType(tag: DW_TAG_typedef, name: "uint8_t", file: !7, line: 185, baseType: !8)
+!7 = !DIFile(filename: "wasm/emscripten/system/include/libc/bits/alltypes.h", directory: "wasm/tests")
+!8 = !DIBasicType(name: "unsigned char", size: 8, align: 8, encoding: DW_ATE_unsigned_char)
+!9 = !{!10}
+!10 = !DISubrange(count: 15)
+!11 = !{i32 2, !"Dwarf Version", i32 4}
+!12 = !{i32 2, !"Debug Info Version", i32 3}
+!13 = !{!"clang version 3.9.0 (trunk 273884) (llvm/trunk 273897)"}
+!14 = distinct !DISubprogram(name: "usage", scope: !1, file: !1, line: 15, type: !15, isLocal: false, isDefinition: true, scopeLine: 15, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !20)
+!15 = !DISubroutineType(types: !16)
+!16 = !{!17, !18}
+!17 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!18 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !19, size: 32, align: 32)
+!19 = !DIBasicType(name: "char", size: 8, align: 8, encoding: DW_ATE_signed_char)
+!20 = !{!21}
+!21 = !DILocalVariable(name: "self", arg: 1, scope: !14, file: !1, line: 15, type: !18)
+!22 = !{i64 0, !"_ZTSFiPcE"}
+!23 = !DIExpression()
+!24 = !DILocation(line: 15, column: 17, scope: !14)
+!25 = !DILocation(line: 16, column: 3, scope: !14)
+!26 = !DILocation(line: 17, column: 3, scope: !14)