1147810 - asm.js: Atomics.load() is incorrectly commoned due to missing effect

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

Consider this test case (i32 is an int32 view, ld is Atomics.load):

1  function f() {
2    var i=0;
3    var j=0;
4    var k = 0;
5    i = ld(i32, 8>>2)|0;
6    k = ld(i32, 12>>2)|0;
7    if ((k|0) != 0)
8      j = ld(i32, 8>>2)|0;
9    return (i+j)|0;
10 }

The code generated is this:

BB #0 [00000] -> #2 -> #1 :: 1 hits
[AsmJSLoadHeap]
movl       0x8(%r15), %edx
[AsmJSLoadHeap]
movl       0xc(%r15), %eax
[CompareAndBranch:ne]
testl      %eax, %eax
jne        .Lfrom66

BB #1 [00000] -> #3 :: 0 hits
[Integer]
xorl       %ecx, %ecx
[Goto]
jmp        .Lfrom87

BB #2 [00000] -> #3 :: 1 hits
[MoveGroup]
movl       %edx, %ecx
[Goto]

BB #3 [00000] :: 1 hits
[MoveGroup]
movl       %ecx, %eax
[AddI]
addl       %edx, %eax
[AsmJSReturn]

Observe that the dependent load from element 2 on line 8 has been commoned with the initial load from element 2 on line 5.

Consider another thread that performs two stores:

  Atomics.store(i32, 8>>2, 7)
  Atomics.store(i32, 12>>2, 1)

If the reading thread observes that i32[3] == 1 then it should also observe in the dependent load that i32[2] == 7 but because of the bug that is not guaranteed since the first read is reused and may have been performed before the store reached that element.

The bug is that the alias set for MAsmJSLoadHeap needs to be AliasSet::Store(AliasSet::AsmJSHeap) when there are barriers involved.  The MIR for plain JS gets this right but (from looking at old patches) it looks like I got this wrong in the MIR for asm.js.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1147810-barriered-load.diff

Comment 2

[Luke Wagner [:luke]]

Presumably stores could also be commoned; perhaps we should add a new AliasSet kind specifically for synchronizing operations.

Comment 3

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #2)
> Presumably stores could also be commoned; perhaps we should add a new
> AliasSet kind specifically for synchronizing operations.

It's possible that that can happen, or that stores can be removed if deemed redundant.  When the code was initially written I had hoped that flagging a store as guarded would not let that happen, but I now think guarded offers less of a guarantee than I want.  (Loads are marked as guarded too.)

I will write a separate test for stores.

A separate alias set is probably the right solution, longer term.  I'll open a separate bug.

Comment 4

[Lars T Hansen [:lth]]

A simple store test suggests no commoning / removal is being performed at present:

    function f() {
	st(i32, 8>>2, 1);
	st(i32, 8>>2, 2);
	return 0;
    }

results in this:

BB #0 [00000] :: 1 hits
[AsmJSStoreHeap]
movl       $0x1, 0x8(%r15)
mfence
[AsmJSStoreHeap]
movl       $0x2, 0x8(%r15)
mfence
[Integer]
xorl       %eax, %eax
[AsmJSReturn]

Comment 5

[Luke Wagner [:luke]]

Well, at present...

Comment 6

[Luke Wagner [:luke]]

And I guess it's not just commoning but movement (wrt control flow) that is a problem.

Comment 7

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #6)

> And I guess it's not just commoning but movement (wrt control flow) that is
> a problem.

Well.  All of these instructions are marked guarded and not movable (and with this patch, as being effectful).  Being guarded means they should not be removed, according to documentation.  Being not movable should mean something too.  But since the alias set is needed to actually enforce non-commoning I'm starting to suspect that those flags don't mean what their names suggest that they mean.

(The aliasing set correction in this patch is needed in any case since an atomic load should always reload its value but that's not what should prevent the instruction from being moved or removed.  So the r? stands but there may be something else that's amiss.)

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8583745 [details] [diff] [review]
bug1147810-barriered-load.diff

Fair enough; perhaps later we'll sort out our annotations more carefully.

Comment 9

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/776865752a67

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/776865752a67