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

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8583745 [details] [diff] [review]
bug1147810-barriered-load.diff
Attachment #8583745 - Flags: review?(luke)

Comment 2

3 years ago
Presumably stores could also be commoned; perhaps we should add a new AliasSet kind specifically for synchronizing operations.
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
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

3 years ago
Well, at present...

Comment 6

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

Comment 7

3 years ago
(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

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

Fair enough; perhaps later we'll sort out our annotations more carefully.
Attachment #8583745 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/776865752a67
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.