1283126 - wasm: Take alignment hints into account when compiling load/store

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

In wasm loads/stores, alignment hints are really just hints: there could be an alignment hint stating that a load/store is aligned, although it would not be aligned during the execution.

So we can only use them when they're saying an access will be unaligned. In this case, we could use a call to memcpy or a simple assembly loop which reads byte by byte and combine the bytes to get the result.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: alignment.patch

This implements a byte load/store loop for accesses declared to be unaligned (note unaligned accesses can still happen, if an access declares itself to be aligned, but it is not dynamically aligned).

I've tested on all WasmLoad/WasmStores (independently of the alignment hint), to make sure this was working correctly and all the tests pass. I've tweaked a few tests to include a less-than-natural alignment hint.

Comment 2

[Lars T Hansen [:lth]]

I've only skimmed this patch but with that caveat: I believe there is better ARM code for this in a patch pending on bug 1277011.  Unless I've completely blown it in that patch one can always do an unaligned 32-bit load in 7 unrolled instructions on ARM, which will be competitive with the loop used here for code size and better for performance.  And it has no memory traffic apart from the individual byte loads.  And it is in masm, so it can be used by the baseline compiler.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Happy to use your code then. I've made this one because it is high priority for shipping wasm on ARM; that being said, if you can isolate the masm code from your patch in bug 1277011 and post it here, that'd be great. I just think the most important thing is to have something in the first place on release, just to prevent many unaligned accesses errors on ARM (which would cause traps, in the current setup).

Comment 4

[Lars T Hansen [:lth]]

No, that's OK, if this needs to be in the code when we branch then just ship it and we can clean up later.  I'm traveling this weekend and I'm unlikely to have something that fits your needs perfectly on the first try.

Anyhow if you're curious it's this patch: https://bug1277011.bmoattachments.org/attachment.cgi?id=8771372.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

More work is needed for int64. I'm on it. Could probably unroll the loop while I'm at it...

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: i64.patch

Additions to make it work for i64 loads and stores.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: folded.patch

Dan, re-requesting review because of new changes:
- int64 unaligned load/store (new LIR nodes, yayyyyyy)
- unrolled the loops as suggested in comment 2 (the code presented there is generalized to handle any signed/unsigned load of <= 4 bytes, and extracted in the masm for maximum reuse)
- everything else stayed the same

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

If the reviewer or somebody else is kind enough to roll in comment fixes and land the patch on my behalf (if it gets r+'d), that'd be great, please :) (I'll be away for a few weeks after tonight)

Comment 9

[Luke Wagner [:luke]]

(I'm happy to help land this after review.)

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8777840 [details] [diff] [review]
folded.patch

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

Looks good!

::: js/src/jit/arm/Lowering-arm.cpp
@@ +666,5 @@
>      MOZ_ASSERT(base->type() == MIRType::Int32);
>  
> +    LAllocation ptr = useRegisterAtStart(base);
> +
> +    if (ins->align() && ins->align() < ins->byteSize()) {

It'd be nice to factor out this predicate into a utility function, like ins->isUnaligned() or so. (and same for the load case above).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f126e84caf09
wasm: Take alignment hints into account when compiling load/store (r=sunfish)

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f126e84caf09