1669179 - MacroAssembler::SupportsFastUnalignedAccesses is ill-defined and insufficiently developed

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

MacroAssembler::SupportsFastUnalignedAccesses is not defined in MacroAssembler.h, but it is used cross-platform and should be defined there.

It is used in wasm code with the understanding that it applies to unaligned integer loads and stores (and I was planning to use it more with this meaning).

It is used in CodeGenerator.cpp with the understanding that it applies to floating point loads (there's a comment that states this, though it looks like the code is more general than the comment).

Wasm code also needs something to talk about both int and FP loads and stores to drive code generation. (Context: bug 1666747.) In practice, it's going to be important to compute the value of the flag at run-time when the hardware is ambiguous about what may or may not be fast (ARM and ARM64), since assuming that unaligned accesses are fast when they are slow, or slow when they are fast, may seriously pessimize wasm compiled code.

Comment 1

[Lars T Hansen [:lth]]

The comment in CodeGenerator.cpp was added by Andre:

changeset:   593243:d36328fd387d
user:        André Bargull <[e-mail]>
date:        Sat May 16 13:46:43 2020 +0000
summary:     Bug 1065894 - Part 5: Inline DataView getter functions. r=jandem

Bug 1065894 is "Optimize native DataView or self host it."

Comment 2

[Lars T Hansen [:lth]]

Furthermore, we have a situation on some platforms where some instructions explicitly support unaligned accesses and thus the predicate might return true, but it is only true if those instructions are generated (and typically never when C++ is used for the access). On ARM, for example, VLD1 is specifically allowed to perform an unaligned access if SCTLR.A=0 (ARMv8 ARM E2.4.2) but VLDR is not. Ditto, LDR is allowed to perform an unaligned access, but (eg) LDM is not. For the DataView load path on ARM, if the predicate returns true, the codegen falls into a path where it will generate code for aligned access (VLDR); if false, it will load via a GPR register using an explicitly unaligned GPR load (LDR + MOV). Ironically, if SCTLR.A=1 then the LDR will trap and be emulated expensively, so this is actually the wrong strategy in that case - a sequence of byte loads would have been better. In the end, therefore, the predicate is tested to basically no effect except to slow down the compiler and make the code harder to read.

(I'm not picking on DataView in particular, I think we're probably not handling the problem coherently in general.)

Comment 3

[Lars T Hansen [:lth]]

Attachment: Bug 1669179 - Clean up predicate for unaligned FP accesses. r?nbp

Several interlinked changes here:

• the predicate SupportsFastUnalignedAccesses is renamed as SupportsFastUnalignedFPAccesses,
  as this is the sense that is needed - we already assume that unaligned integer accesses
  are supported.
• DataView accessors that use this predicate are rewritten to test that the access type
  is an FP access before using this predicate
• In Wasm, the uses of this predicate only pertain to when FP accesses are used to
  implement inlined/unrolled memory.copy and memory.fill, so move the predicate so
  that it guards this case only, not all cases.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b97b0ab411f
Clean up predicate for unaligned FP accesses.  r=nbp

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/5b97b0ab411f