Closed Bug 1330942 Opened 3 years ago Closed 3 years ago

Wasm: Make MemoryAccessDesc::isUnaligned smarter, or move it to the platform layer

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(1 file)

Current definition:

    bool isUnaligned() const { return align() && align() < byteSize(); }

But (1) single-byte accesses are always aligned, and the ARM backend now has to know this specially to avoid emitting suboptimal code, and (2) on ARM, double loads are executed (by the chip) as two word loads and the double only needs to be 4-byte aligned, but this is not reflected in the predicate nor in the current ARM back-end.

Arguably on x86 every access is aligned (but the predicate is not used there).

Hannes noted the predicate should be made smarter, but now that I think about it, it's possible it should just move to the platform layer and be removed from common code.  Once the load/store refactoring is done (bug 1312751) and Rabaldr no longer needs it it will only be used in the ARM and MIPS back-ends.
Great point regarding ARM double loads!  Agreed this should be moved into the platform.  For (1), though, wouldn't align() = 1, byteSize() = 1 and thus isUnaligned() = false?
Excellent point about (1), you're right of course.
Let's try to get this done for FF54.
Priority: -- → P2
I'm agnostic about where we put the platform-specific IsUnassigned() but the platform assembler seemed reasonable, at least.

(The MIPS code hasn't been buildable for a while so the MIPS changes here are strictly best-effort / courtesy.)
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Attachment #8828297 - Flags: review?(luke)
Comment on attachment 8828297 [details] [diff] [review]
bug1330942-isUnaligned-per-platform.patch

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

Thanks!

::: js/src/jit/mips-shared/Assembler-mips-shared.h
@@ +1523,5 @@
>        : Instruction(op | RS(rs) | RT(rt) | off.encode(6) | ff)
>      { }
>  };
>  
> +bool IsUnaligned(const wasm::MemoryAccessDesc& access) {

nit: { on new line and you'll need 'inline' for this to build
Attachment #8828297 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1a68a78656619e068a0252155083c762ccd9a4
Bug 1330942 - move MemoryAccessDesc::isUnaligned to the ARM/MIPS platform layer. r=luke
https://hg.mozilla.org/mozilla-central/rev/7f1a68a78656
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.