1423577 - Odin: Remove asm.js atomics

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Lars T Hansen [:lth]]

Now that wasm atomics have landed and we have removed the crufty callout code for ARMv6 we can definitely change Odin so that it generates standard wasm atomic opcodes instead of our internal opcodes.

We can then remove all support for the internal atomic opcodes from WasmIonCompile.

The only thing that looks at all difficult is that the asm.js atomic store returns a result, but the wasm atomic store does not.  This should be easy to account for.

(Whether we should spend the time to do this I don't know - there's little code that supports our internal opcodes, most code is shared between asm.js atomics and wasm atomics.)

Comment 1

[Lars T Hansen [:lth]]

The other option is to just remove atomics from asm.js; at the moment they are effectively not-shipped, as SAB is disabled.

Re the point I made above:

> (Whether we should spend the time to do this I don't know - there's little code that supports our internal opcodes, most code is shared between asm.js atomics and wasm atomics.)

there's probably enough asm.js-only code that it's worthwhile doing something here, and there are some additional paths in signal handling that we would probably be happy to remove.

Comment 2

[Luke Wagner [:luke]]

I think it's fine to just remove once we capture the value of the additional asm.js-only Atomics tests.  But you wrote these tests, so you're the better judge of the value (and which subset of tests are valuable).

Comment 3

[Lars T Hansen [:lth]]

OK.  I'll put test triage / translation high on my to-do list so that we don't block on that.

Comment 4

[Lars T Hansen [:lth]]

Atomics are used by these tests in js/src/jit-test/tests/asm.js:

 gating.js - not relevant for wasm, wasm has a different gating mechanism
 sta-transition.js - basically another gating test, not relevant for wasm
 testAsmJSWasmMixing.js - and another gating test, irrelevant once asm.js atomics go away
 testAtomic-effect.js - a regression test for an assembler bug, porting this is probably not meaningful
 testAtomics.js - these were previously ported and generalized as ../wasm/atomic.js 
 testBug1155176.js - verification bug for asm.js, not relevant to wasm
 testBug1164391.js - bounds checking regression test, subsumed by ../wasm/atomic.js
 testBug1183060.js - 16-bit regression test cases, subsumed by ../wasm/atomic.js

In js/src/jit-test/tests/ion:

 bug1266768.js - fuzz bug test case, not relevant for wasm

In js/src/jit-test/tests/atomics:

 optimization-tests.js - has an asm.js test case with atomics that tests back-to-back operations,
     results need to be inspected manually, not much of a test case...

It does not seem like there are any asm.js tests in js/src/tests/.

In sum, I don't think any of these tests need porting to wasm, we can just nuke them when we nuke asm.js atomics.

Comment 5

[Lars T Hansen [:lth]]

Renaming bug to capture actual intent.

Comment 6

[Luke Wagner [:luke]]

Thanks for doing that audit and agreed with your assessment.  Seems like there is no reason to delay removing asm.js atomics.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1423577-rm-asmjs-atomics.patch

Removes everything I could find (though see the next patch too).

As a side note, I think we can now in principle enable Rabaldr for asm.js code, if we're so inclined...

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1423577-rm-signal-handling-fallback.patch

This removes the fallback that's used if lookupTrap() fails to find information.  We can't land this until wasm atomics opt into some kind of bounds checking, either by recording trap information or by emitting explicit bounds checks.

Comment 9

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=346c811424771009395a66df440fa8ee1e64b902

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8997383 [details] [diff] [review]
bug1423577-rm-asmjs-atomics.patch

Review of attachment 8997383 [details] [diff] [review]:
-----------------------------------------------------------------

It's interesting to see that, compared to SIMD.js, this was rather a smaller removal.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8997384 [details] [diff] [review]
bug1423577-rm-signal-handling-fallback.patch

Review of attachment 8997384 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, exactly!  Actually, if we're soon to be conditionally enabling wasm threads, given the problem the out-of-bounds stub explained in GenerateGenericMemoryAccessTrap()'s comment, I think we need to enable proper traps for wasm atomics.  I think it's not as anymore since we don't care about framePushed, just pcOffset+bytecodeOffset, just tedious to hunt them all down.

::: js/src/wasm/WasmGenerator.cpp
@@ -516,5 @@
>          debugTrapCodeOffset_ = codeRange.begin();
>          break;
> -      case CodeRange::OutOfBoundsExit:
> -        MOZ_ASSERT(!linkDataTier_->outOfBoundsOffset);
> -        linkDataTier_->outOfBoundsOffset = codeRange.begin();

I think you can delete LinkDataTier::outOfBoundsOffset too which will trigger another small transitive wave of removals.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e1f880b5f88
Remove asm.js atomics support.  r=luke

Comment 13

[Lars T Hansen [:lth]]

OK, given that we're going to block on handling signals for atomics, I'm going to fork this bug and move the second patch elsewhere.

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/7e1f880b5f88