[PATCH] D101736: [WebAssembly] Allow DBG_VALUE after terminator in MachineVerifier

Sun May 2 19:43:00 PDT 2021

aheejin created this revision.
aheejin added reviewers: dschuff, aardappel, yurydelendik.
Herald added subscribers: wingo, ecnelises, sunfish, hiraditya, jgravelle-google, sbc100.
aheejin requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

When a stackified variable has an associated `DBG_VALUE` instruction,
DebugFixup pass adds a `DBG_VALUE` instruction after the stackified
value's last use to clear the variable's debug range info. But when the
last use instruction is a terminator, it can cause a verification
failure (when run with `-verify-machineinstrs`) because there are no
instructions allowed after a terminator.

For example:

  %myvar = ...
  DBG_VALUE target-index(wasm-operand-stack), $noreg, !"myvar", ...
  BR_IF 0, %myvar, ...
  DBG_VALUE $noreg, $noreg, !"myvar", ...

In this test, `%myvar` is stackified, so the first `DBG_VALUE`
instruction's first operand has changed to `wasm-operand-stack` to
denote it. And an additional `DBG_VALUE` instruction is added after its
last use, `BR_IF`, to signal variable `myvar` is not in the operand
stack anymore. But because the `DBG_VALUE` instruction is added after
the `BR_IF`, a terminator, it fails MachineVerifier.

I experimented on whether we could add the `DBG_VALUE` before the
terminator in this kind of case, but it loses info in the resulting
debug info. If we do that in the example above, the result will be:

  %myvar = ...
  DBG_VALUE target-index(wasm-operand-stack), $noreg, !"myvar", ...
  DBG_VALUE $noreg, $noreg, !"myvar", ...
  BR_IF 0, %myvar, ...

Now the debug info for `myvar` has changed twice, rendering the first
`DBG_VALUE` meaningless. In this case this info is dropped because its
range is empty.

So this CL adds an exception for wasm in MachineVerifier's terminator
check. The reason this does not happen with other targets is, I think,
in general when `DBG_VALUE` has to be inserted right after an
instruction that writes to a register. If that register used to contain
a variable's value, that variable's `DBG_VALUE` has to be cleared. And
also if the newly written value is a value for another variable,
`DBG_VALUE` for that variable has to be added. But terminator
instructions don't generally write to registers, obviating the need to
add `DBG_VALUE`s right after them.
But wasm is special in this way, because uses, not defs, can change the
value stack location's contents. Our terminator instructions, such as
`BR_IF` don't write to registers but uses registers, which can be
stackified, changing the value stack and thus possibly invalidating some
variables' value in previous `DBG_VALUE`s, so they have to be cleared by
additional `DBG_VALUE`s after the terminator in that case.

I am not very familiar with how debug info works, so I might be
mistaken, and please correct me if so.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101736

Files:
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/test/CodeGen/WebAssembly/stackified-debug.ll


Index: llvm/test/CodeGen/WebAssembly/stackified-debug.ll
===================================================================

--- llvm/test/CodeGen/WebAssembly/stackified-debug.ll
+++ llvm/test/CodeGen/WebAssembly/stackified-debug.ll
@@ -1,4 +1,4 @@
-; RUN: llc < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs < %s | FileCheck %s
 
 ; Input C code:
 
@@ -42,8 +42,6 @@
 ; CHECK: 	.int8	159                       # DW_OP_stack_value
 
 
-
-
 source_filename = "stackified.c"
 target triple = "wasm32-unknown-unknown"
 
@@ -57,10 +55,25 @@
   ret void, !dbg !22
 }
 
-declare i32 @input()
+; DebugFixup pass adds a DBG_VALUE instruction to clear the debug value range of
+; "myvar" (!27) after BR_IF instruction in the entry BB. Even though it is
+; generally not allowed to have more instructions after a terminator, this
+; special case should not crash MachineVerifier.
+define void @dbg_value_after_terminator(i32 %a, i32 %b) !dbg !23 {
+entry:
+  %cmp = icmp ne i32 %a, %b, !dbg !28
+  call void @llvm.dbg.value(metadata i1 %cmp, metadata !27, metadata !DIExpression(DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 8, DW_ATE_unsigned, DW_OP_stack_value)), !dbg !28
+  br i1 %cmp, label %bb.1, label %bb.0, !dbg !28
 
-declare !dbg !4 void @output(i32, i32)
+bb.0:                              ; preds = %lor.lhs.false111
+  unreachable
 
+bb.1:                                       ; preds = %lor.lhs.false111
+  ret void
+}
+
+declare i32 @input()
+declare !dbg !4 void @output(i32, i32)
 declare void @llvm.dbg.value(metadata, metadata, metadata)
 
 !llvm.dbg.cu = !{!0}
@@ -90,3 +103,10 @@
 !20 = !DILocation(line: 5, column: 11, scope: !12)
 !21 = !DILocation(line: 6, column: 3, scope: !12)
 !22 = !DILocation(line: 7, column: 1, scope: !12)
+
+!23 = distinct !DISubprogram(name: "dbg_value_after_terminator", unit:!0, type:!24)
+!24 = !DISubroutineType(types: !{})
+!25 = distinct !DILexicalBlock(scope: !23)
+!26 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!27 = !DILocalVariable(name: "myvar", scope: !25, type: !26)
+!28 = !DILocation(line: 0, scope: !25)
Index: llvm/lib/CodeGen/MachineVerifier.cpp
===================================================================
--- llvm/lib/CodeGen/MachineVerifier.cpp
+++ llvm/lib/CodeGen/MachineVerifier.cpp
@@ -794,6 +794,18 @@
     lastIndex = idx;
   }
 
+  // WebAssemblyDebugFixup pass can generate a DBG_VALUE instruction after a
+  // terminator, in case the terminator instruction consumes a stack operand.
+  // Consuming a stack operand changes the contents of the stack, so a DBG_VALUE
+  // instruction is necessary to terminate the associated variable's range.
+  // The DBG_VALUE instruction takes a form of
+  // DBG_VALUE $noreg, $noreg, !"variable", ...
+  if (TM->getTargetTriple().isWasm() && FirstTerminator && MI->isDebugValue() &&
+      MI->getOperand(0).isReg() && MI->getOperand(1).isReg() &&
+      !MI->getOperand(0).getReg().isValid() &&
+      !MI->getOperand(1).getReg().isValid())
+    return;
+
   // Ensure non-terminators don't follow terminators.
   if (MI->isTerminator()) {
     if (!FirstTerminator)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D101736.342301.patch
Type: text/x-patch
Size: 3131 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210503/9d6327b3/attachment.bin>
