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

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

(Blocks: 1 bug)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 1

2 years ago
Created attachment 8775963 [details] [diff] [review]
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.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8775963 - Flags: review?(sunfish)

Comment 2

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

Comment 3

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

2 years ago
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.

Updated

2 years ago
Attachment #8775963 - Flags: review?(sunfish) → review+
(Assignee)

Comment 5

2 years ago
More work is needed for int64. I'm on it. Could probably unroll the loop while I'm at it...
(Assignee)

Comment 6

2 years ago
Created attachment 8777474 [details] [diff] [review]
i64.patch

Additions to make it work for i64 loads and stores.
(Assignee)

Comment 7

2 years ago
Created attachment 8777840 [details] [diff] [review]
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
Attachment #8775963 - Attachment is obsolete: true
Attachment #8777474 - Attachment is obsolete: true
Attachment #8777840 - Flags: review?(sunfish)
(Assignee)

Comment 8

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

2 years ago
(I'm happy to help land this after review.)
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).
Attachment #8777840 - Flags: review?(sunfish) → review+

Comment 11

2 years ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f126e84caf09
wasm: Take alignment hints into account when compiling load/store (r=sunfish)

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f126e84caf09
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Depends on: 1293312
You need to log in before you can comment on or make changes to this bug.