Odin: refactor in preparation for WebAssembly decoder

RESOLVED FIXED in Firefox 45

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

2 years ago
The rough plan is:
 - factor out common wasm types (value type, expression type, opcodes) and push asm.js specific details into AsmJSValidate.cpp
 - factor out wasm::Module from AsmJSModule so it's possible to create a wasm::Module without JS source and other asm.js-specific bits
 - define AsmJSCompile.(h,cpp) solely in terms of the wasm common types/module so that it can be used as the common backend of both wasm and asm.js (and then rename to WasmCompile.(h,cpp))
 - move stub generation and other compilation machinery from AsmJSValidate.cpp into WasmCompile.cpp so that WasmCompile can generate a complete wasm::Module
 - generalize asm.js cache lookup/store so that it isn't dependent on jschars

With this, it should be possible to start writing a simple decoder for the emerging wasm binary format that reuses the rest of the asm.js pipeline.
(Assignee)

Comment 1

2 years ago
Created attachment 8686865 [details] [diff] [review]
Part 1 - wasm types

This patch does the first bullet and depends on bug 1181612.  It's unfortunately rather big, but mostly boilerplate.  A few notes:
 - all the new wasm stuff is moved into namespace wasm
 - in general, VarType is replaced by wasm::ValType and RetType is replaced by wasm::ExprType (see description in WasmIR.h comments); this is most of the patch.  Quite a few redundant enums were able to be removed (AsmType, AsmJSCoercion, AsmJSModule::ReturnType).  Signature is replaced with wasm::Sig.
 - AsmJSType and AsmJSNumLit survive but are moved into AsmJSValidate.cpp and un-prefixed.
 - AsmFunction is renamed to wasm::FuncIR and, to avoid ambiguity within AsmJSValidate, 'func' means the MV::Func and 'funcIR' means the FuncIR. In WasmCompile.cpp, there is only FuncIR, so it's always named 'func'.
 - Several AsmFunction fields were removed by observing that they were only needed in the validator, where MV::Func was available.
 - FunctionCompileResults::codeRange_ was removed, storing FunctionLabels instead. This ends up putting the right info in the right place and simplifying quite a few things (e.g., allowing the removal of FunctionCodeRange).
 - FunctionLabels::overflowLabel was removed since masm.asmOnStackOverflowLabel() could be used instead
 - The patch adds a 'HashSet<LifoSig*> sigs_' to share signatures when they get Lifo-allocated.  This could be pulled into a separate patch with some pain, but the logic is pretty self contained to 2 functions and a new hash policy.

 28 files changed, 2318 insertions(+), 2499 deletions(-)
Assignee: nobody → luke
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8686865 - Attachment description: wasm-ir → Part 1 - wasm types
Attachment #8686865 - Flags: review?(benj)
(Assignee)

Comment 2

2 years ago
Created attachment 8687449 [details] [diff] [review]
Part 1: wasm types and Wasm.h

Actually, I'll probably iterate on this a bit while I work on the next patches.
Attachment #8686865 - Attachment is obsolete: true
Attachment #8686865 - Flags: review?(benj)
(Assignee)

Updated

2 years ago
Attachment #8687449 - Attachment description: wasm-h → Part 1: wasm types and Wasm.h
(Assignee)

Updated

2 years ago
Depends on: 1227767
(Assignee)

Comment 3

2 years ago
Created attachment 8691653 [details] [diff] [review]
Part 1: wasm types and Wasm.h / WasmIR.h

In addition to above notes:
 - the patch refactors how offsets are passed around in a way that really simplifies some things: now instead of Labels, uint32_t offsets are used
 - the stack overflow check generation is moved out of AsmJSFrameIterator.cpp and into CodeGenerator.cpp where it's a lot more natural
 - the sig pool is not in this patch anymore
 - AsmJSCompile.(h,cpp) -> WasmIonCompile.(h,cpp) ("Ion" in preparation for baseline)
Attachment #8687449 - Attachment is obsolete: true
Attachment #8691653 - Flags: review?(benj)
(Assignee)

Comment 4

2 years ago
Created attachment 8691654 [details] [diff] [review]
Part 1 - wasm types / headers
Attachment #8691653 - Attachment is obsolete: true
Attachment #8691653 - Flags: review?(benj)
Attachment #8691654 - Flags: review?(benj)
(Assignee)

Comment 5

2 years ago
Created attachment 8691656 [details] [diff] [review]
Part 2 - simplify AsmJSModule

This patch simplifies how globals are added to AsmJSModule, relaxing the ordering restriction and allowing globals, func-ptr-tables and exits to all interleave.  Widespread use of global indices are replaced with global-data-offsets (which is what is usually wanted).  This allows various simplifications.
Attachment #8691656 - Flags: review?(benj)
(Assignee)

Comment 6

2 years ago
Created attachment 8691663 [details] [diff] [review]
wasm-generator

This patch:
 - moves all the stub generate out into a more-encapsulated WasmStubs.(h,cpp)
 - factors out WasmGenerator.(h,cpp) which handle all masm business as well as the sequential/parallel Ion compilation
 - moves all masm logic out of AsmJSValidate.cpp into WasmGenerator.cpp
 - changes func-ptr-tables to not use RelativeLinks but instead a separate array of offsets stored in the AsmJSModule::FuncPtrTable.  This saves ~.5mb (for a Unity-sized app) since RelativeLink stores 2 uint32_ts for each element instead of just 1 and simplifies the interactions with funcEntryOffsets which was moved into ModuleGenerator from ModuleValidator

This ends up simplifying AsmJSValidate.cpp quite a bit (which is now "only" 7kloc).  The main goal is for the WasmDecoder to be able to reuse WasmGenerator.
Attachment #8691663 - Flags: review?(benj)
(Assignee)

Comment 7

2 years ago
Created attachment 8691664 [details] [diff] [review]
Part 3 - factor out WasmGenerator
Attachment #8691663 - Attachment is obsolete: true
Attachment #8691663 - Flags: review?(benj)
Attachment #8691664 - Flags: review?(benj)
(Assignee)

Updated

2 years ago
Attachment #8691654 - Attachment description: wasm-h → Part 1 - wasm types / headers
(Assignee)

Updated

2 years ago
Attachment #8691656 - Attachment description: simplify-module-building → Part 2 - simplify AsmJSModule
(Assignee)

Updated

2 years ago
Attachment #8691664 - Attachment description: wasm-generator → Part 3 - factor out WasmGenerator
(Assignee)

Comment 8

2 years ago
Created attachment 8691803 [details] [diff] [review]
Part 1 - wasm types / headers

Updated with fixes for static analysis and other things found by try.
Attachment #8691654 - Attachment is obsolete: true
Attachment #8691654 - Flags: review?(benj)
(Assignee)

Updated

2 years ago
Attachment #8691803 - Attachment description: 8691654: Part 1 - wasm types / headers → Part 1 - wasm types / headers
Attachment #8691803 - Flags: review?(benj)
(Assignee)

Comment 9

2 years ago
Created attachment 8691804 [details] [diff] [review]
Part 2 - simplify AsmJSModule
Attachment #8691656 - Attachment is obsolete: true
Attachment #8691656 - Flags: review?(benj)
Attachment #8691804 - Flags: review?(benj)
(Assignee)

Comment 10

2 years ago
Created attachment 8691805 [details] [diff] [review]
Part 3 - factor out WasmGenerator
Attachment #8691664 - Attachment is obsolete: true
Attachment #8691664 - Flags: review?(benj)
Attachment #8691805 - Flags: review?(benj)
(Assignee)

Comment 11

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fc9dd674931
Comment on attachment 8691653 [details] [diff] [review]
Part 1: wasm types and Wasm.h / WasmIR.h

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

sigh, i just realized i was reviewing the wrong file

::: js/src/asmjs/AsmJSFrameIterator.h
@@ +158,5 @@
>  
> +struct AsmJSOffsets
> +{
> +    AsmJSOffsets(uint32_t begin = 0, uint32_t end = 0)
> +      : begin(begin), end(end) {}

nit: here and below, can you please put the {} on the next line?

@@ +160,5 @@
> +{
> +    AsmJSOffsets(uint32_t begin = 0, uint32_t end = 0)
> +      : begin(begin), end(end) {}
> +
> +    uint32_t begin;

To make it less confusing, could we call it profilingEntry? Or explicitly document that this is the begin of the profiling entry?

::: js/src/asmjs/Wasm.h
@@ +34,5 @@
> +
> +enum class ValType : uint8_t
> +{
> +    I32,
> +    I64,

w00t

@@ +92,5 @@
> +    explicit Val(uint32_t i32) : type_(ValType::I32) { u.i32_ = i32; }
> +    explicit Val(uint64_t i64) : type_(ValType::I64) { u.i64_ = i64; }
> +    explicit Val(float f32) : type_(ValType::F32) { u.f32_ = f32; }
> +    explicit Val(double f64) : type_(ValType::F64) { u.f64_ = f64; }
> +    explicit Val(const I32x4& i32x4) : type_(ValType::I32x4) { memcpy(u.i32x4_, i32x4, sizeof(u.f32x4_)); }

nit: sizeof(u.i32x4_)

::: js/src/asmjs/WasmIonCompile.h
@@ +58,1 @@
>      mozilla::Maybe<FunctionCompileResults> results;

I had put the mozilla::Maybe at the top because its sizeof is >= sizeof(FunctionCompileResults), so it was more efficient in terms of packing to put it before the pointers. Mind to put it back at the top, or after the lifo?

::: js/src/jit/IonTypes.h
@@ +265,5 @@
>          Undefined = -1
>      };
>  
> +    typedef int32_t I32x4[4];
> +    typedef float F32x4[4];

Could we use int32x4_t and float32x4_t here, or will it conflict with included headers?

@@ +348,5 @@
>          MOZ_ASSERT(defined());
>          return type_;
>      }
>  
> +    const I32x4& asInt32x4() const {

I32X4 is a pointer type, so you're returning a reference to a pointer type here. However, it makes the API nice to read, even though the reference is useless here, so we can keep it.

::: js/src/jit/MIRGenerator.h
@@ +41,1 @@
>                   bool usesSignalHandlersForAsmJSOOB = false);

This could benefit from renaming.

@@ -201,5 @@
> -    Label* outOfBoundsLabel_;
> -    // Label where we should jump in asm.js mode, in the case where we have an
> -    // invalid conversion or a loss of precision (when converting from a
> -    // floating point SIMD type into an integer SIMD type).
> -    Label* conversionErrorLabel_;

Wow, good catch for those two. Wonder how I've missed that.

::: js/src/jsopcode.cpp
@@ -231,5 @@
> -                fprintf(stdout, "--- Asm.js Module ---\n");
> -
> -                for (size_t i = 0; i < module.numFunctionCounts(); i++) {
> -                    jit::IonScriptCounts* counts = module.functionCounts(i);
> -                    DumpIonScriptCounts(&sprinter, counts);

Aren't there users of the IonScriptCounts? Maybe nbp's work on code coverage is actually using them...

::: js/src/vm/HelperThreads.h
@@ +276,3 @@
>       * -1 if no function has failed.
>       */
> +    const wasm::FuncIR* wasmFailedFunction;

While you're here, can you group all these attribute members with the other wasm related members (worklist/finished jobs)?
Comment on attachment 8691803 [details] [diff] [review]
Part 1 - wasm types / headers

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

Nice! Thank you for getting rid of so many types, it makes thinking much easier. The code looks much cleaner and tighter (short names ftw). Mostly optional nits / renaming questions and typos, so r+.

::: js/src/asmjs/AsmJSFrameIterator.h
@@ +159,5 @@
> +struct AsmJSOffsets
> +{
> +    MOZ_IMPLICIT AsmJSOffsets(uint32_t begin = 0,
> +                              uint32_t end = 0)
> +      : begin(begin), end(end) {}

nit: here and below, can you please put the {} on the next line?

@@ +161,5 @@
> +    MOZ_IMPLICIT AsmJSOffsets(uint32_t begin = 0,
> +                              uint32_t end = 0)
> +      : begin(begin), end(end) {}
> +
> +    uint32_t begin;

To make it less confusing, could we call it profilingEntry? Or explicitly document that this is the begin of the profiling entry?

::: js/src/asmjs/AsmJSLink.cpp
@@ +148,3 @@
>              break;
> +          case ValType::F64:
> +            *(double*)datum = v.f64();

We might as well use memcpy for all these cases.

::: js/src/asmjs/AsmJSModule.cpp
@@ -2169,5 @@
> -    // compiled in, which both affects code speed and uses absolute addresses
> -    // that can't be serialized. (This is separate from normal profiling and
> -    // requires an addon to activate).
> -    if (module.numFunctionCounts())
> -        return JS::AsmJSCache_Disabled_JitInspector;

Maybe AsmJSCache_Disabled_JitInspecter can be deleted as well? And there's probably a static string somewhere mapping the enum value to a textual reason.

::: js/src/asmjs/AsmJSModule.h
@@ +331,5 @@
>                           PropertyName* maybeFieldName,
> +                         wasm::MallocSig&& sig)
> +         : name_(name),
> +           maybeFieldName_(maybeFieldName),
> +           sig_(Move(sig))

nit: mozilla::Move (we're in a header file)

@@ +435,5 @@
>          enum Kind { Function, Entry, JitFFI, SlowFFI, Interrupt, Thunk, Inline };
>  
>          CodeRange() {}
> +        CodeRange(Kind kind, AsmJSOffsets offsets);
> +        CodeRange(Kind kind, AsmJSProfilingOffsets offsets);

AsmJSProfilingOffsets inherits from AsmJSOffsets. Could it be that a compiler be confused and choose the wrong ctor?

@@ +436,5 @@
>  
>          CodeRange() {}
> +        CodeRange(Kind kind, AsmJSOffsets offsets);
> +        CodeRange(Kind kind, AsmJSProfilingOffsets offsets);
> +        CodeRange(AsmJSExit::BuiltinKind builtin, AsmJSProfilingOffsets offsets);

Can we make sure that we're using the right ctor by making Kind an enum class above? There could be a confusion between the two ctors if AsmJSExit::BuiltinKind is not an enum class as well.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +642,5 @@
> +        MOZ_CRASH("bad literal");
> +    }
> +};
> +
> +// Respresents the type of a general asm.js expression.

typo: respresents

@@ +830,5 @@
> +        else if (isDouble())
> +            return ValType::F64;
> +        else if (isInt32x4())
> +            return ValType::I32x4;
> +        return ValType::F32x4;

in this function: no else after return
+ maybe MOZ_ASSERT(isFloat32x4()); at the end?

@@ +2370,1 @@
>          hasAlreadyReturned_ = true;

Seems that we could use a Maybe<ExprType> for storing the return type, and get rid of hasAlreadyReturned_ here.

@@ +5985,5 @@
>  {
> +    if (expr->isKind(PNK_CALL)) {
> +        Type _;
> +        return CheckCoercedCall(f, expr, ExprType::Void, &_);
> +    }

I preferred it the old way, this way adds a type declaration and {} for the if body. Could we keep it?

@@ +8128,5 @@
> +    masm.assertStackAlignment(ABIStackAlignment);
> +    masm.call(AsmJSImmPtr(AsmJSImm_ReportOverRecursed));
> +    masm.jump(throwLabel);
> +
> +

nit: blank line

::: js/src/asmjs/Wasm.h
@@ +94,5 @@
> +    explicit Val(uint32_t i32) : type_(ValType::I32) { u.i32_ = i32; }
> +    explicit Val(uint64_t i64) : type_(ValType::I64) { u.i64_ = i64; }
> +    explicit Val(float f32) : type_(ValType::F32) { u.f32_ = f32; }
> +    explicit Val(double f64) : type_(ValType::F64) { u.f64_ = f64; }
> +    explicit Val(const I32x4& i32x4) : type_(ValType::I32x4) { memcpy(u.i32x4_, i32x4, sizeof(u.f32x4_)); }

nit: sizeof(u.i32x4_)

@@ +112,5 @@
> +// The ExprType enum represents the type of a WebAssembly expression or return
> +// value and may either be a value type or void. A future WebAssembly extension
> +// may generalize expression types to instead be a list of value types (with
> +// void represented by the empty list). For now it's easier to have a flat enum
> +// and be explicit about converions to/from value types.

nit: converions => conversions

@@ +178,5 @@
> +    ExprType ret_;
> +
> +  protected:
> +    Sig(AllocPolicy alloc = AllocPolicy()) : args_(alloc) {}
> +    Sig(Sig&& rhs) : args_(Move(rhs.args_)), ret_(rhs.ret_) {}

I think the static analysis might want these two be explicit?

@@ +179,5 @@
> +
> +  protected:
> +    Sig(AllocPolicy alloc = AllocPolicy()) : args_(alloc) {}
> +    Sig(Sig&& rhs) : args_(Move(rhs.args_)), ret_(rhs.ret_) {}
> +    explicit Sig(ArgVector&& args, ExprType ret) : args_(Move(args)), ret_(ret) {}

This one doesn't need to be explicit, though it's safe to keep the keyword here.

::: js/src/asmjs/WasmIonCompile.cpp
@@ +2930,5 @@
>  
> +    TempAllocator alloc(&lifo);
> +    JitContext jitContext(args.runtime, &alloc);
> +
> +    JitCompileOptions options;

Can options stay const?

::: js/src/asmjs/WasmIonCompile.h
@@ +58,1 @@
>      mozilla::Maybe<FunctionCompileResults> results;

I had put the mozilla::Maybe at the top because its sizeof is >= sizeof(FunctionCompileResults), so it was more efficient in terms of packing to put it before the pointers. Mind to put it back at the top, or after the lifo?

@@ +63,5 @@
>          runtime(nullptr),
>          func(nullptr)
>      { }
>  
> +    void init(JSRuntime* rt, wasm::FuncIR* func) {

Could the func be const as well?

::: js/src/devtools/rootAnalysis/annotations.js
@@ -39,5 @@
>          return true;
>  
> -    var CheckCallArgs = "AsmJSValidate.cpp:uint8 CheckCallArgs(FunctionValidator*, js::frontend::ParseNode*, (uint8)(FunctionValidator*,js::frontend::ParseNode*,js::wasm::Type)*, js::wasm::Signature*)";
> -    if (name == "checkArg" && caller == CheckCallArgs)
> -        return true;

Just realized there is still a mention of the ModuleCompiler in this file, feel free to delete it as well.

::: js/src/jit/IonTypes.h
@@ +265,5 @@
>          Undefined = -1
>      };
>  
> +    typedef int32_t I32x4[4];
> +    typedef float F32x4[4];

Could we use int32x4_t and float32x4_t here (it'd be very cute), or will it conflict with included headers?

@@ +348,5 @@
>          MOZ_ASSERT(defined());
>          return type_;
>      }
>  
> +    const I32x4& asInt32x4() const {

I32X4 is a pointer type, so you're returning a reference to a pointer type here. However, it makes the API nice to read, even though the reference is useless here, so we can keep it.

::: js/src/jit/MIRGenerator.h
@@ +41,1 @@
>                   bool usesSignalHandlersForAsmJSOOB = false);

This could benefit from renaming.

::: js/src/vm/HelperThreads.h
@@ +276,1 @@
>       * -1 if no function has failed.

This comment needs an update.

@@ +276,3 @@
>       * -1 if no function has failed.
>       */
> +    const wasm::FuncIR* wasmFailedFunction;

While you're here, can you group these two attribute members with the other wasm related members (worklist/finished jobs)?
Attachment #8691803 - Flags: review?(benj) → review+
Comment on attachment 8691804 [details] [diff] [review]
Part 2 - simplify AsmJSModule

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

Much clearer and nice to read, thank you!

::: js/src/asmjs/AsmJSModule.cpp
@@ +504,5 @@
>              return true;
>      }
>  
>      // The exit may have become optimized while executing the FFI.
> +    auto& exit = module.exit(exitIndex);

I don't clearly remember how ended the discussions on dev platform about the use of auto, but my rule of thumb was to use "auto" iff the type is already explicited in the same line. For instance, auto* ptr = lifoAlloc.new_<MyType>();
This use case doesn't fall under this category, so can you replace it by the actual type, please?

@@ +766,2 @@
>  
> +    for (auto& exit : exits_)

ditto

::: js/src/asmjs/AsmJSModule.h
@@ +855,5 @@
> +    /*************************************************************************/
> +    // These functions may only be called before finish():
> +
> +  private:
> +    bool allocateGlobalBytes(uint32_t bytes, uint32_t align, uint32_t* globalDataOffset) {

Can we MOZ_ASSERT(!isFinished()); here? (or not, as all callers will)

@@ +1037,5 @@
>      bool addBuiltinThunkCodeRange(AsmJSExit::BuiltinKind builtin, AsmJSProfilingOffsets offsets) {
>          return builtinThunkOffsets_.append(offsets.begin) &&
>                 codeRanges_.append(CodeRange(builtin, offsets));
>      }
>      bool addExit(unsigned ffiIndex, unsigned* exitIndex) {

Can we MOZ_ASSERT(!isFinished()); here?

@@ +1187,5 @@
> +        static_assert(jit::AsmJSActivationGlobalDataOffset == 0, "");
> +        static_assert(jit::AsmJSHeapGlobalDataOffset == jit::AsmJSActivationGlobalDataOffset + sizeof(void*), "");
> +        static_assert(jit::AsmJSNaN64GlobalDataOffset == jit::AsmJSHeapGlobalDataOffset + sizeof(uint8_t*), "");
> +        static_assert(jit::AsmJSNaN32GlobalDataOffset == jit::AsmJSNaN64GlobalDataOffset + sizeof(double), "");
> +        static_assert(sInitialGlobalDataBytes == jit::AsmJSNaN32GlobalDataOffset + sizeof(float), "");

Please fill these static_assert text just with some hints: "global data starts with the activation ptr", "then the heap ptr", "then f64 NaN", etc.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +3570,5 @@
>  {
>      RootedPropertyName name(f.cx(), lhs->name());
>  
>      size_t opcodeAt = f.tempOp();
> +    size_t patchAt = f.temp32();

I'd keep the former name (opcodeAt is also getting patched, as it's a temp op).
Attachment #8691804 - Flags: review?(benj) → review+
Comment on attachment 8691805 [details] [diff] [review]
Part 3 - factor out WasmGenerator

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

First batch of comments, I'll finish the review tomorrow.

::: js/src/asmjs/AsmJSModule.cpp
@@ +1575,5 @@
> +    size_t size = relativeLinks.sizeOfExcludingThis(mallocSizeOf) +
> +                  absoluteLinks.sizeOfExcludingThis(mallocSizeOf) +
> +                  funcPtrTables.sizeOfExcludingThis(mallocSizeOf);
> +
> +    for (auto& table : funcPtrTables)

I would not use auto here.

@@ +1846,5 @@
>      }
>  
>      // Update all the addresses in the function-pointer tables to point to the
>      // profiling prologues:
> +    for (auto& funcPtrTable : staticLinkData_.funcPtrTables) {

ditto

::: js/src/asmjs/AsmJSValidate.cpp
@@ +1222,5 @@
>      ModuleValidator(ExclusiveContext* cx, AsmJSParser& parser)
>        : cx_(cx),
>          parser_(parser),
> +        mg_(cx),
> +        validationLifo_(4096),

Can you hoist that constant into a global static const, please?

@@ +7009,5 @@
>      if (!moduleObj)
>          return false;
>  
> +    // The module function dynamically links the AsmJSModule when called and
> +    // genenerates a set of functions wrapping all the exports.

typo: genenenenenenerates

::: js/src/asmjs/WasmGenerator.cpp
@@ +47,5 @@
> +// ModuleGenerator
> +
> +ModuleGenerator::ModuleGenerator(ExclusiveContext* cx)
> +  : cx_(cx),
> +    lifo_(4 * 1024),

Can you hoist this constant somewhere in this file?

@@ +89,5 @@
> +                HelperThreadState().wait(GlobalHelperThreadState::CONSUMER);
> +            }
> +        }
> +
> +        MOZ_ASSERT(HelperThreadState().wasmCompilationInProgress == true);

nit: MOZ_ASSERT(X == true); => MOZ_ASSERT(X);

@@ +119,5 @@
> +
> +    if (!tasks_.initCapacity(numTasks))
> +        return false;
> +    for (size_t i = 0; i < numTasks; i++)
> +        tasks_.infallibleEmplaceBack(64 * 1024, args());

Can you also hoist this constant, please?

@@ +276,5 @@
> +    funcEntryOffsets_[func.index()] = codeRange.entry();
> +
> +    // Keep a record of slow functions for printing in the final console message.
> +    unsigned totalTime = func.generateTime() + results.compileTime();
> +    if (totalTime >= 250) {

How about promoting 250 to be a static const in SlowFunctions?

@@ +294,5 @@
> +    return true;
> +}
> +
> +CompileArgs
> +ModuleGenerator::args()

This method could be const, right?

::: js/src/asmjs/WasmIR.h
@@ +413,5 @@
>      Bytecode bytecode_;
>      unsigned generateTime_;
>  
>    public:
> +    explicit FuncIR(LifoAlloc& alloc, PropertyName* name, unsigned line, unsigned column)

(no need to be explicit anymore)

::: js/src/asmjs/WasmStubs.cpp
@@ +1210,5 @@
> +            return false;
> +
> +        if (!GenerateExceptionExit(masm, AsmJSImm_OnOutOfBounds, &onThrow,
> +                                   masm.asmOnOutOfBoundsLabel(), &offsets))
> +            return false;

{} for multi-line conditions

@@ +1221,5 @@
> +    if (masm.asmOnConversionErrorLabel()->used()) {
> +        AsmJSOffsets offsets;
> +        if (!GenerateExceptionExit(masm, AsmJSImm_OnImpreciseConversion, &onThrow,
> +                                   masm.asmOnConversionErrorLabel(), &offsets))
> +            return false;

ditto
Comment on attachment 8691805 [details] [diff] [review]
Part 3 - factor out WasmGenerator

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

Okay, assuming this is mostly code motion, the review I've made yesterday and the one today should be enough for spotting the big things. Please address my previous batch of comments in addition to these, and r=me. Nice work!

::: js/src/asmjs/AsmJSModule.h
@@ +271,5 @@
>      };
>  
>      class Exit
>      {
> +        wasm::MallocSig sig_;

Is it really necessary to store the signature here? It seems that this information is only used for compilation. Unless you're planning for wasm, which might store signatures of exits directly in the wasm IR?

::: js/src/asmjs/WasmCompileArgs.h
@@ +24,2 @@
>  namespace jit {
>      class CompileRuntime;

This declaration isn't needed anymore.

::: js/src/vm/HelperThreads.h
@@ -210,5 @@
>          uint32_t n = numWasmFailedJobs;
>          numWasmFailedJobs = 0;
>          return n;
>      }
> -    void noteWasmFailure(const wasm::FuncIR* func) {

I guess you're removing the argument because it doesn't add much value to know which function OOMed + the WasmGenerator doesn't have a way to pass an error message to its owner? Sounds fine by me.

@@ -221,5 @@
>      bool wasmFailed() const {
>          return bool(numWasmFailedJobs);
>      }
> -    void resetWasmFailureState() {
> -        numWasmFailedJobs = 0;

OK to remove this function, but for sanity, could you assert in WasmGenerator.cpp (maybe init()) that MOZ_ASSERT(!HelperThreadState().wasmFailed()); ?
Attachment #8691805 - Flags: review?(benj) → review+
(Assignee)

Comment 17

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #12)
Thanks for the quick review and all the good feedback.  Addressed them all, with these exceptions:

> >      mozilla::Maybe<FunctionCompileResults> results;
> 
> I had put the mozilla::Maybe at the top because its sizeof is >=
> sizeof(FunctionCompileResults), so it was more efficient in terms of packing
> to put it before the pointers. Mind to put it back at the top, or after the
> lifo?

The reason for putting it at the bottom is b/c 'results' has a logically shorter lifetime than the LifoAlloc.  When FCR gets a TempAllocator in the next patch, that becomes a hard requirement.  Also, since there are only a handful of these tasks alive, the number of bytes shouldn't be significant.

> > +    typedef int32_t I32x4[4];
> > +    typedef float F32x4[4];
> 
> Could we use int32x4_t and float32x4_t here, or will it conflict with
> included headers?

I'm worried it would be confused with something in a standard library (either conflicts or by the reader).

> > +    const I32x4& asInt32x4() const {
> 
> I32X4 is a pointer type, so you're returning a reference to a pointer type
> here. However, it makes the API nice to read, even though the reference is
> useless here, so we can keep it.

The array/ptr rules are tricky, and I'm not an expert on them, but I don't think it's quite that simple: they are indeed separate types, just with goofy conversion rules.  Array types are straight-up disallowed as return types (so it's an error to take out the & here).  For parameters, the array types get straight-up erased to pointer types, such that:
  void foo(int x[4]) {}
  void foo(int *x) {}
is a redefinition error, not two overloads and
  void foo(int x[4]) {}
  void bar() { int x[5]; foo(x); }
type-checks successfully.  However, slapping a & on an array type treats them as real array types such that:
  void foo(int (&x)[4]) {}
  void bar() { int x[5]; foo(x); }
is a type-check error and you can overload on different array lengths, etc.

So I think the use of & here is the ideal thing.

> >                   bool usesSignalHandlersForAsmJSOOB = false);
> 
> This could benefit from renaming.

There's a bunch of AsmJS names in MIRGenerator, so I was thinking I'd hold off on mass renaming until a later point.

> Aren't there users of the IonScriptCounts? Maybe nbp's work on code coverage
> is actually using them...

I hadn't heard anything about this; I thought this was just for JIT inspector and other one-off tools that weren't officially supported and may not be actively used.  Without tests, I feel weird shuffling the code around knowing it could well be broken.  Seems easy enough to add back later if someone wants it.
(Assignee)

Comment 18

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #16)
Thanks for all the reviews and great comments.

> Is it really necessary to store the signature here? It seems that this
> information is only used for compilation. Unless you're planning for wasm,
> which might store signatures of exits directly in the wasm IR?

Yes, ultimately I think we will need these for wasm.  We *would* need it at runtime to compile the Ion exits, however, we observe the dynamic type of the js::Value and assume it stays the same.  But that is pretty sketchy, so I thought this would be the better fix.

> I guess you're removing the argument because it doesn't add much value to
> know which function OOMed + the WasmGenerator doesn't have a way to pass an
> error message to its owner? Sounds fine by me.

Yup
Comment on attachment 8691803 [details] [diff] [review]
Part 1 - wasm types / headers

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

::: js/src/asmjs/Wasm.h
@@ +98,5 @@
> +    explicit Val(const I32x4& i32x4) : type_(ValType::I32x4) { memcpy(u.i32x4_, i32x4, sizeof(u.f32x4_)); }
> +    explicit Val(const F32x4& f32x4) : type_(ValType::F32x4) { memcpy(u.f32x4_, f32x4, sizeof(u.f32x4_)); }
> +
> +    ValType type() const { return type_; }
> +    bool isSimd() const { return type_ == ValType::I32x4 || type_ == ValType::F32x4; }

return IsSimdType(type());

::: js/src/asmjs/WasmIR.h
@@ +289,5 @@
> +};
> +
> +enum class I32X4 : uint8_t
> +{
> +    // Common opcodes

The SIMD.js spec currently has 7 SIMD types, and we will probably implement Float64x2 and Bool64x2 too. I am assuming that WebAssembly will eventually adopt these SIMD types?

This approach of explicitly listing all opcodes for all result types becomes quite repetitive and error prone with that many types with almost identical operations. I wonder if there is a better way?
(Assignee)

Comment 20

2 years ago
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #19)
> return IsSimdType(type());

Good catch!

> This approach of explicitly listing all opcodes for all result types becomes
> quite repetitive and error prone with that many types with almost identical
> operations. I wonder if there is a better way?

Yes, I think this current design is more of a path-dependent artifact and I'm definitely in favor of refactoring to share what makes sense.

Comment 21

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9239605f27a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d1bdaedc485
https://hg.mozilla.org/integration/mozilla-inbound/rev/88256698e1a5

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9239605f27a8
https://hg.mozilla.org/mozilla-central/rev/0d1bdaedc485
https://hg.mozilla.org/mozilla-central/rev/88256698e1a5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1229698
You need to log in before you can comment on or make changes to this bug.