Closed
Bug 1079361
Opened 10 years ago
Closed 10 years ago
SIMD: OdinMonkey support for load, store
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: sunfish, Assigned: bbouvier)
References
Details
Attachments
(4 files, 1 obsolete file)
38.95 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
44.26 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
28.89 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
5.06 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
With [0], the SIMD.js spec now has SIMD methods for load and store which don't depend on Float32x4Array or DataView, and so should be much more suitable for asm.js validation and optimization. We already have macroassembler/assembler interfaces for loading and storing int32x4 and float32x4, so it it looks like the main tasks here are asm.js validation, and either figuring out how to fit this within MAsmJSLoadHeap/MAsmJSStoreHeap or doing something new. [0] https://github.com/johnmccutchan/ecmascript_simd/pull/65
Reporter | ||
Updated•10 years ago
|
Summary: SIMD: load, store → SIMD: OdinMonkey support for load, store
Assignee | ||
Comment 1•10 years ago
|
||
I've got a local WIP patch which knows how to perform (non optimized) loads and put special values for OOB accesses (NaN x4 for float32x4 and 0 x4 for int32x4).
Assignee: nobody → benj
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
This implements load/store on int32x4/float32x4. loadX/loadXY/loadXYZ and store equivalents are either to be implemented, or won't be at all (we're currently discussing this). This patch is bigger than expected. Doesn't handle yet all cases of optimizations for the pointer value, but works good enough for testing. Would be nice to already get some feedback, as it's quite big.
Attachment #8513671 -
Flags: feedback?(luke)
Comment 3•10 years ago
|
||
Comment on attachment 8513671 [details] [diff] [review] WIP Review of attachment 8513671 [details] [diff] [review]: ----------------------------------------------------------------- Looking good!
Attachment #8513671 -
Flags: feedback?(luke) → feedback+
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8513671 -
Attachment is obsolete: true
Attachment #8516710 -
Flags: review?(luke)
Assignee | ||
Comment 5•10 years ago
|
||
Two remarks: - I wasn't sure whether we'd need enter/leave Heap expressions in Odin. Do we? As call arguments are evaluated left-to-right, the heap could change in a call inside the evaluation of the index expr. I put them just in case, let me know if they're superfluous. - On x64 and x86, AsmJSHeapAccesses which are stores also need to save the view type. This is needed for x86 which adjusts the length when patching the bounds checks, for both loads and stores.
Attachment #8516812 -
Flags: review?(luke)
Comment 6•10 years ago
|
||
Comment on attachment 8516710 [details] [diff] [review] 1) Refactor AsmJSHeapAccess Review of attachment 8516710 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/AsmJSSignalHandlers.cpp @@ +384,5 @@ > } > > # if defined(JS_CODEGEN_X64) > static void > +SetRegisterToCoercedUndefined(CONTEXT *context, AsmJSHeapAccess::ViewType viewType, AnyRegister reg) Make sure you try-server this with "-p all". In particular, I think you might be missing the OSX paths, but I could be wrong. ::: js/src/jit/MIR.h @@ +11668,5 @@ > }; > > class MAsmJSHeapAccess > { > + AsmJSHeapAccess::ViewType viewType_; To shorten all these types, could you 'typedef AsmJSHeapAccess::ViewType ViewType;' (protected: so that Load/Store can use it as well)? @@ +11690,5 @@ > setMovable(); > + switch (vt) { > + case AsmJSHeapAccess::Float32: setResultType(MIRType_Float32); break; > + case AsmJSHeapAccess::Float64: setResultType(MIRType_Double); break; > + default: setResultType(MIRType_Int32); break; Can you explicitly enumerate them to avoid the default (here and everywhere else you switch on ViewType)? In some cases, the default cases are pre-existing. ::: js/src/jit/shared/Assembler-shared.h @@ +701,5 @@ > + Int32 = Scalar::Int32, > + Uint32 = Scalar::Uint32, > + Float32 = Scalar::Float32, > + Float64 = Scalar::Float64, > + Uint8Clamped = Scalar::Uint8Clamped, Can you remove Uint8Clamped? @@ +709,5 @@ > uint32_t offset_; > #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64) > uint8_t cmpDelta_; // the number of bytes from the cmp to the load/store instruction > uint8_t opLength_; // the length of the load/store instruction > + ViewType viewType_; Could this be a :8 bitfield? @@ +734,5 @@ > AsmJSHeapAccess(uint32_t offset, uint8_t after, uint32_t cmp = NoLengthCheck) > : offset_(offset), > cmpDelta_(cmp == NoLengthCheck ? 0 : offset - cmp), > opLength_(after - offset), > + viewType_(Float64), How about ViewType(-1)? I was wondering "is this an overload just for float64?" before I got it :) @@ +753,5 @@ > bool hasLengthCheck() const { return cmpDelta_ > 0; } > void *patchLengthAt(uint8_t *code) const { return code + (offset_ - cmpDelta_); } > unsigned opLength() const { return opLength_; } > bool isLoad() const { return loadedReg_ != UINT8_MAX; } > + ViewType viewType() const { return viewType_; } Perhaps MOZ_ASSERT(viewType_ != ViewType(-1));?
Attachment #8516710 -
Flags: review?(luke) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8516812 [details] [diff] [review] 2) SIMD.*.load/store Review of attachment 8516812 [details] [diff] [review]: ----------------------------------------------------------------- Nice job! ::: js/src/asmjs/AsmJSModule.cpp @@ +807,5 @@ > + // Float32x4 and Int32x4 accesses don't clear low bits of their > + // pointer, so we need to explicitly bound check that we won't > + // perform a partial heap access. I.e., we don't want > + // ptr + Simd128DataSize > heapLength > + // i.e. ptr >= heapLength + 1 - Simd128DataSize Instead of describing this as a special-case of the X4 types, what if you did the length-adjustment for all element types (using a generalization of TypedArrayShift to derive the element size from the viewType)? This will pave the way for if we are ever able to remove the alignment requirement from loads/stores (i can has int32.load?). ::: js/src/asmjs/AsmJSValidate.cpp @@ +5252,5 @@ > + *viewType = AsmJSHeapAccess::Int32x4; > + else if (retType.isFloat32x4()) > + *viewType = AsmJSHeapAccess::Float32x4; > + else > + MOZ_CRASH("unexpected SIMD type"); I wish we could pass around AsmJSSimdType in all these places we pass around a Type-which-isSimd. Here's an idea: we could change: Type retType = global->simdOperationType(); in CheckSimdOperationCall to: AsmJSSimdType opType = global->simdOperationType(); and then change all the SIMD operations to take an 'AsmJSSimdType opType' instead of 'Type retType'? For one thing, 'retType' isn't really an accurate name since the return type of the expression is not always retType (the comparison case returns int32x4). For another, using the less-precise type Type causes us to define simdToScalarType and simdToCoercedScalarType as member functions on type that are only valid when isSimd instead of having non-member SimdToScalarType()/SimdToCoercedScalarType() functions. We could have a 'AsmJSSimdType Type::simdType()' which was useful after you've checked isSimd() (dynamically, as part of type checking). What do you think? If you agreed, this change could be separated out into a separate patch (on top of this one to avoid rebase pain). ::: js/src/jit/shared/CodeGenerator-x86-shared.cpp @@ +346,5 @@ > bool > CodeGeneratorX86Shared::visitOutOfLineLoadTypedArrayOutOfBounds(OutOfLineLoadTypedArrayOutOfBounds *ool) > { > + static const SimdConstant NaNX4 = SimdConstant::SplatX4(float(GenericNaN())); > + static const SimdConstant ZeroX4 = SimdConstant::SplatX4(0); I'd just stick 'em inline in their respective uses.
Attachment #8516812 -
Flags: review?(luke) → review+
Assignee | ||
Comment 8•10 years ago
|
||
This implements your suggestion. There are a few places where it's not obvious whether we should use Type or AsmJSSimdType (in particular in the type checking function objects). It's not consistent between the different checkers, but it's logical: for a given function object, the most used form (among Type or AsmJSSimdType) gets stored in the function object, and the other one gets constructed when used. Bleh.
Attachment #8521471 -
Flags: review?(luke)
Comment 9•10 years ago
|
||
Comment on attachment 8521471 [details] [diff] [review] Use AsmJSSimdType rather than Type in SIMD type checking in asm.js Review of attachment 8521471 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/asmjs/AsmJSValidate.cpp @@ +4352,5 @@ > lane = LaneW; > else > return f.fail(base, "dot access field must be a lane name (x, y, z, w) or signMask"); > > + *type = SimdToScalarType(baseType.simdType()); I hadn't noticed that this function had only one use; could you inline it into this function? @@ +5303,5 @@ > return f.fail(view, "expected Uint8Array view as SIMD.*.store first argument"); > } > > *needsBoundsCheck = NEEDS_BOUNDS_CHECK; > + *viewType = SimdTypeToViewType(opType); Similarly, I think it'd be fine to keep the switch inline since there is only one use.
Attachment #8521471 -
Flags: review?(luke) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Luke, this fixes a few comments you've addressed when reviewing patch 2, plus fixes the mysterious issue on Mac OS X debug-only. For history: when accessing the address indexed at -1 (i.e. HeapBase + 0xFFFFFFFF) on Mac OS X, the sigsegv handler was faulting at HeapBase + 0xFFFFFFFF + 5, which wasn't in bounds in the debug-only range checking carried out in the Mach handler. Then the Mach handler would return false, which is converted into a failure message sent to the Mach system, which causes a segfault. This is fixed by adding a small amount of space in the mapped memory region, which will also be needed anyways for bug 915157. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cb0307bc9aee
Attachment #8526198 -
Flags: review?(luke)
Comment 11•10 years ago
|
||
Comment on attachment 8526198 [details] [diff] [review] Fix to be folded into patch 2) Review of attachment 8526198 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/AsmJSModule.cpp @@ +794,5 @@ > const jit::AsmJSHeapAccess &access = heapAccesses_[i]; > + if (access.hasLengthCheck()) { > + // An access is out-of-bounds iff > + // ptr + data-type-byte-size > heapLength > + // i.e. ptr >= heapLength + 1 - data-type-byte-size Nice explanation. Perhaps additionally point out that codegen uses >= which is why we have to switch. ::: js/src/asmjs/AsmJSValidate.h @@ +56,5 @@ > #ifdef JS_CPU_X64 > // On x64, the internal ArrayBuffer data array is inflated to 4GiB (only the > // byteLength portion of which is accessible) so that out-of-bounds accesses > // (made using a uint32 index) are guaranteed to raise a SIGSEGV. > +// SIMD accesses and mask optimizations might also try to access a few bytes Perhaps replace "SIMD" by just "unaligned" (we may get unaligned accesses of other data types in the future).
Attachment #8526198 -
Flags: review?(luke) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Fixed, and with a nice rebasing over lth's SAB patches :) remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/74527e0493c5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/39e6791cc5c5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d29ca3d8c2
Comment 13•10 years ago
|
||
Sorry had to back this out for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4105583&repo=mozilla-inbound
Assignee | ||
Comment 14•10 years ago
|
||
Forgot to update the atomic's parts in arm to use AsmJSHeapAccess as well, duh. Take 2 (and hopefully ultimate take): remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6d91e75f9b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/973929d1f314 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/abe3335741bd
https://hg.mozilla.org/mozilla-central/rev/ca6d91e75f9b https://hg.mozilla.org/mozilla-central/rev/973929d1f314 https://hg.mozilla.org/mozilla-central/rev/abe3335741bd
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•