Fixes https://bugs.llvm.org/show_bug.cgi?id=39158 and regression caused by
D49034. Though it is possible the problem was existed before and was exposed by
additional DBG_VALUEs.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
debug instructions can interleaved between any instruction, e.g. i32.wrap/i64 and i32.eqz in this case. Which causes block to start from i32.eqz and causing the validation error as mentioned in https://bugs.llvm.org/show_bug.cgi?id=39158
| lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp | ||
|---|---|---|
| 313 ↗ | (On Diff #168139) | How about if (std::prev(I)->isDebugInstr() || std::prev(I)->isPosition()) ? The same for the TRY placement. Because labels also can go anywhere and are not stackified. |
| lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp | ||
|---|---|---|
| 313 ↗ | (On Diff #168139) | Added "std::prev(I)->isPosition()". Thanks. |
| test/CodeGen/WebAssembly/cfg-stackify-dbg-skip.ll | ||
|---|---|---|
| 7 ↗ | (On Diff #168181) | Maybe a little comment about what this test is about would be good for future readers..? Like, why block should be placed there and where a debug instruction was in MIR. (Speaking of which, an MIR test would probably be better, but I guess this is also fine) |
Comment Actions
Is there any way to we can make this test case more focused?
How does the output of the test differ without this fix?
Comment Actions
It inserts "block" between "i32.wrap/i64" and "i32.eqz", because there DBG_VALUE after "i32.wrap/i64"
Comment Actions
In MIR tests,
before (wrong):
%18:i32 = I32_WRAP_I64 ... ; defines %18 DBG_VALUE ... BLOCK BR_UNLESS ... %18 ... ; uses %18
after (right):
BLOCK %18:i32 = I32_WRAP_I64 ... ; defines %18 DBG_VALUE ... BR_UNLESS ... %18 ... ; uses %18
So I32_WRAP_I64 is a child of BR_UNLESS in the stackified form, but the loop stops at DBG_VALUE without checking I32_WRAP_I64 and places BLOCK in the middle of the stackified instruction sequence.
I think the way we can make this test case clearer or more focused is do an MIR test, which would show the instruction sequence containing DBG_VALUE, which disappears in .s file, as I suggested above.
Comment Actions
I'm sorry, it turns out this MIR test doesn't work for now, because the target-specific state in the target-specific MachineFunctionInfo subclasses isn’t serialized at the moment. Continuing our discussion from IRC, the reason why this MIR test succeeds even without your patch is, [[ https://github.com/llvm-mirror/llvm/blob/f590076c467cd7ea2a349c81ae02bafe71591496/lib/Target/WebAssembly/WebAssemblyUtilities.cpp#L102 | MFI.isVRegStackified(Reg) ]] in [[ https://github.com/llvm-mirror/llvm/blob/f590076c467cd7ea2a349c81ae02bafe71591496/lib/Target/WebAssembly/WebAssemblyUtilities.cpp#L93-L103 | WebAssembly::isChild ]] always returns false, because [[ https://github.com/llvm-mirror/llvm/blob/f590076c467cd7ea2a349c81ae02bafe71591496/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h#L38-L44 | VRegStackified ]] variable accessed in [[ https://github.com/llvm-mirror/llvm/blob/f590076c467cd7ea2a349c81ae02bafe71591496/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h#L96-L101 | WebAssemblyFunctionInfo::isVRegStackified ]] is now empty. This is supposed to be populated in RegStackify pass, but MIR file cannot serialize information in target-dependent subclasses of MachineFunctionInfo, so we can't pass that info to CFGStackify here.
The serialization of target-dependent MachineFunctionInfo is something I'd like to implement someday for my own purpose unless someone does that by then, but I don't think this patch should be blocked on it anyway. One way we possibly still do an MIR test is to pass multiple passes to llc. AFAIK you can pass multiple -run-pass flags to llc, like llc -run-pass wasm-reg-stackify -run-pass wasm-reg-coloring .... Maybe we can start from mir just before the RegStackify pass and make llc run all passes from RegStackify to CFGStackify. I think it's worth trying. If this fails for some other reason, we can bring back .ll test case...
Either way, could you minimize the test case and elaborate little more on 'before' and 'after' results? I'm sorry for the hassle. Thank you.
Comment Actions
I had not luck with making a mir test this way: it fails at wasm-explicit-locals pass. Locks like I need to list all passes starting from basic one -- the same thing running .ll would do.
I reverted back to the .ll test, though I start checking MIR before and after wasm-cfg-stackify pass and location of the DBG_VALUE.
Comment Actions
Thank you. Could you minimize the test so it becomes something like a unit test..? This test looks rather like generated. And I'm not sure we need all those debug values either..
| test/CodeGen/WebAssembly/cfg-stackify-dbg-skip.ll | ||
|---|---|---|
| 6 ↗ | (On Diff #168294) | How about 'stackified expression' instead of 'logical expression'? |
Comment Actions
Would bugpoint help here? Its specifically designed to minimize test case I think although I don't have a lot of experience with it myself.
Comment Actions
I run delta tool -- it took 20 lines off. I need to figure out what is encoded in the DIDerivedType's to further reduce the test.
Comment Actions
Thank you! From what you uploaded, I tried to tidy it a bit more. What do you think?
This is basically the same as what you uploaded, but I simplified variable names a bit, and deleted a few more unused lines, and added a bit more comments.
And I don't think we need the BEFORE check... what I meant by the 'before' check in the comment above was, before your fix, the wrong code sequence generated was like that. I wasn't suggesting to compare before and after CFGStackify.
; RUN: llc %s -stop-after wasm-cfg-stackify -o - | FileCheck %s
; The test ensures "block" instruction is not inserted in the middle of a group
; of instructions that form a stackified expression when DBG_VALUE is present
; among them.
; CHECK: body:
; CHECK: BLOCK
; <-- Stackified expression starts
; CHECK-NEXT: GET_LOCAL_I64
; CHECK-NEXT: I32_WRAP_I64
; CHECK-NEXT: DBG_VALUE
; <-- BLOCK should NOT be placed here!
; CHECK-NEXT: BR_UNLESS
; <-- Stackified expression ends
target triple = "wasm32-unknown-unknown"
define void @foo(i64 %arg) {
start:
%val = trunc i64 %arg to i32
%cmp = icmp eq i32 %val, 0
call void @llvm.dbg.value(metadata i32 %val, metadata !46, metadata !DIExpression()), !dbg !105
br i1 %cmp, label %bb2, label %bb1
bb1: ; preds = %start
call void @bar()
br label %bb2
bb2: ; preds = %bb1, start
ret void
}
declare void @bar()
declare void @llvm.dbg.value(metadata, metadata, metadata)
!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!33}
!0 = distinct !DICompileUnit(language: DW_LANG_Rust, file: !6, producer: "clang LLVM (rustc version 1.30.0-dev)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !2)
!2 = !{}
!6 = !DIFile(filename: "<unknown>", directory: "")
!22 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "&str", file: !6, size: 64, align: 32, elements: !{}, identifier: "111094d970b097647de579f9c509ef08")
!33 = !{i32 2, !"Debug Info Version", i32 3}
!35 = distinct !DILexicalBlock(scope: !37, file: !6, line: 357, column: 8)
!37 = distinct !DISubprogram(name: "foobar", linkageName: "_fooba", scope: !38, file: !6, line: 353, type: !39, isLocal: true, isDefinition: true, scopeLine: 353, flags: DIFlagPrototyped, isOptimized: true, unit: !0, templateParams: !2, retainedNodes: !42)
!38 = !DINamespace(name: "ptr", scope: null)
!39 = !DISubroutineType(types: !2)
!42 = !{!46}
!46 = !DILocalVariable(name: "z", scope: !35, file: !6, line: 357, type: !22, align: 4)
!105 = !DILocation(line: 357, column: 12, scope: !35)Comment Actions
And I don't think we need the BEFORE check... what I meant by the 'before' check in the comment above was, before your fix, the wrong code sequence generated was like that.
CHECK-BEFORE is a sanity check in case if some of the passes decide to rearrange the instructions or remove DBG_VALUE. Maybe it was an overkill.
Thank you for the reduced test case.
This comment was removed by aheejin.