1972116 - wasm loop unroller: correctly handle dependences through memory

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Yury Delendik (:yury)]

After fixing bug 1966549 I noticed that few seconds in the game, the program hangs but allows the script to stop. In the debugger, the following machine code can be observed, which is compiled wasm code:

    ...
    0x115bab6ea728: fcmp   s4, s0
    0x115bab6ea72c: b.ge   0x115bab6ea75c
    0x115bab6ea730: ldr    s0, 0x115bab6ea9c0
    0x115bab6ea734: fsub   s0, s3, s0
    0x115bab6ea738: str    s0, [x0, #0x1c0]
    0x115bab6ea73c: nop    
    0x115bab6ea740: str    s0, [x0, #0x1c0]
    0x115bab6ea744: str    s0, [x0, #0x1c0]
    0x115bab6ea748: ldr    x1, [x28, #0xc0]
    0x115bab6ea74c: ldr    w16, [x1, #0x40]
    0x115bab6ea750: cbnz   w16, 0x115bab6f4c50
    0x115bab6ea754: str    s0, [x0, #0x1c0]
->  0x115bab6ea758: b      0x115bab6ea740
    0x115bab6ea75c: ldr    s0, [x0, #0x1c0]
    0x115bab6ea760: fsub   s2, s2, s0
    ....

Notice that the code/loop at 0x115bab6ea740 - 0x115bab6ea758 is doing nothing useful. Disabling javascript.options.wasm_unroll_loops fixes the issue.

Website https://eaglercraft.com/mc/1.12.2-wasm/

Comment 1

[Yury Delendik (:yury)]

Attachment: test case extracted from large application wasm

Comment 2

[Julian Seward [:jseward]]

Attachment: The Badness

The loop in question looks OK immediately after unrolling. But after the
post-unroll cleanup (GVN + dead block removal), all the side exits have
disappeared, causing it to be an infinite loop. Also almost all of the other
contents of the original loop have been removed. See attached log.

Comment 3

[Julian Seward [:jseward]]

Attachment: Minimal test case

Immediately after unrolling, each copy of the original loop body has its own
copy of the struct.get, and the associated f32.lt, f32.add and struct.set that
that feeds. After the post-unroll cleanup, everything except the struct.set
disappears in the unrolled iterations; the struct.get, f32.lt and f32.add
appear appear only in the peeled iteration.

This would be consistent with the struct.gets in the unrolled iterations having
been GVNd out on the basis that they are duplicates of the one in the peeled
iteration -- which they are not, because the struct.set invalidates the old
value each time round the loop.

Comment 4

[Julian Seward [:jseward]]

Attachment: Even more minimal test case

This one is also runnable. On baseline it prints 4444200, which is correct;
for Ion it prints 360, which definitely isn't.

Comment 5

[Julian Seward [:jseward]]

Attachment: Preliminary fix

When combined with the test case in comment 4, this would probably
be a complete fix.

Comment 6

[Julian Seward [:jseward]]

Attachment: Bug 1972116 - wasm loop unroller: correctly handle dependences through memory. r=iain.

When duplicating MIR nodes in the original loop body, the load dependency field
was not being updated.

It is necessary for the load dependency field of a duplicated node to be
updated to point at the corresponding duplicated store node in the same
iteration-copy.

The result was that the load dependency fields ended up still pointing at the
store node in the original loop, which eventually becomes the peeled iteration,
outside the loop. Post-unrolling GVN could then common up all the loads and
replace them with the value loaded in the peeled iteration, so that the loaded
value was, incorrectly, the value stored by the first iteration of the
(original) loop.

This patch fixes it in a straightforward way, and adds a regression test.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/da8c3761b6cd
https://hg.mozilla.org/integration/autoland/rev/9e20e0a11b4c
wasm loop unroller: correctly handle dependences through memory.  r=iain.

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/9e20e0a11b4c