Page MenuHomePhabricator

[WebAssembly] Ignore DBG_VALUE in WebAssemblyCFGStackify pass when looking for block start
ClosedPublic

Authored by yurydelendik on Oct 3 2018, 11:00 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

yurydelendik created this revision.Oct 3 2018, 11:00 AM

Why should we skip debug instructions when searching for block start?

Why should we skip debug instructions when searching for block start?

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

aheejin added inline comments.Oct 3 2018, 2:08 PM
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.

Skipping labels

yurydelendik marked an inline comment as done.Oct 3 2018, 2:57 PM
yurydelendik added inline comments.
lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
313 ↗(On Diff #168139)

Added "std::prev(I)->isPosition()". Thanks.

aheejin added inline comments.Oct 3 2018, 3:12 PM
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)

yurydelendik marked an inline comment as done.

Add test comment

yurydelendik marked an inline comment as done.Oct 3 2018, 3:24 PM
aheejin accepted this revision.Oct 3 2018, 3:30 PM
This revision is now accepted and ready to land.Oct 3 2018, 3:30 PM
sbc100 added a comment.Oct 3 2018, 3:35 PM

Is there any way to we can make this test case more focused?

How does the output of the test differ without this fix?

How does the output of the test differ without this fix?

It inserts "block" between "i32.wrap/i64" and "i32.eqz", because there DBG_VALUE after "i32.wrap/i64"

Is there any way to we can make this test case more focused?

How does the output of the test differ without this fix?

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.

Maybe it's better to do an MIR test anyway.

aheejin requested changes to this revision.Oct 3 2018, 3:53 PM
This revision now requires changes to proceed.Oct 3 2018, 3:53 PM

Add mir test

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, MFI.isVRegStackified(Reg) in WebAssembly::isChild always returns false, because VRegStackified variable accessed in 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.

Revert test to .ll form, but ensure valid input before wasm-cfg-stackify pass

yurydelendik retitled this revision from [WebAssembly] Ignore DBG_VALUE in WebAssemblyCFGStacify pass when looking for block start to [WebAssembly] Ignore DBG_VALUE in WebAssemblyCFGStackify pass when looking for block start.Oct 4 2018, 8:00 AM

.. 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...

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.

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'?

sbc100 added a comment.Oct 4 2018, 9:36 AM

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.

Minimize test case; fix its comment

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.

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.

yurydelendik marked an inline comment as done.Oct 4 2018, 12:14 PM

Further reduction of cfg-stackify-dbg-skip.ll

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)

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.

Further reduced by @aheejin test case

This comment was removed by aheejin.
aheejin accepted this revision.Oct 4 2018, 4:30 PM

Thank you!

This revision is now accepted and ready to land.Oct 4 2018, 4:30 PM
This revision was automatically updated to reflect the committed changes.