Skip to content

Commit

Permalink
Bug 1668373 - wasm: Split CompilerEnvironment from ModuleEnvironment.…
Browse files Browse the repository at this point in the history
… r=lth

ModuleEnvironment contains a CompilerEnvironment pointer, which describes the compiler selection,
tiering, and whether debugging is enabled. None of this information is needed for validation,
so wasm::Validate() has to create a synthetic CompilerEnvironment which is never read. This
isn't a large issue, but indicates ModuleEnvironment is doing too many things.

This commit removes CompilerEnvironment from ModuleEnvironment and pipes it
through the compiler pipeline. This is fairly straightforward except for:
  * wasm::Validate() no longer needs to create a synthetic compiler env
  * DecodeModuleEnvironment() used to invoke moduleEnv.compilerEnv.computeParameters(d)
    after the preamble was decoded. I believe this was needed for handling the GC opt-in
    section, which is no longer used. I moved this call to computeParameters() to after
    all callers of DecodeModuleEnvironment().
  * I added an assertion in IonCompileFunctions/CraneliftCompileFunctions that they are
    not being invoked for debugged modules.

Differential Revision: https://phabricator.services.mozilla.com/D91995
  • Loading branch information
eqrion committed Oct 5, 2020
1 parent ab77889 commit fb950b7
Show file tree
Hide file tree
Showing 12 changed files with 90 additions and 79 deletions.
4 changes: 2 additions & 2 deletions js/src/wasm/AsmJS.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1359,7 +1359,7 @@ class MOZ_STACK_CLASS ModuleValidatorShared {
arrayViews_(cx),
compilerEnv_(CompileMode::Once, Tier::Optimized, OptimizedBackend::Ion,
DebugEnabled::False),
moduleEnv_(&compilerEnv_, FeatureArgs(), ModuleKind::AsmJS) {
moduleEnv_(FeatureArgs(), ModuleKind::AsmJS) {
compilerEnv_.computeParameters();
moduleEnv_.minMemoryLength = RoundUpToNextValidAsmJSHeapLength(0);
}
Expand Down Expand Up @@ -2123,7 +2123,7 @@ class MOZ_STACK_CLASS ModuleValidator : public ModuleValidatorShared {
return nullptr;
}

ModuleGenerator mg(*args, &moduleEnv_, nullptr, nullptr);
ModuleGenerator mg(*args, &moduleEnv_, &compilerEnv_, nullptr, nullptr);
if (!mg.init(asmJSMetadata_.get())) {
return nullptr;
}
Expand Down
43 changes: 25 additions & 18 deletions js/src/wasm/WasmBaselineCompile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3221,6 +3221,7 @@ class BaseCompiler final : public BaseCompilerInterface {
};

const ModuleEnvironment& moduleEnv_;
const CompilerEnvironment& compilerEnv_;
BaseOpIter iter_;
const FuncCompileInput& func_;
size_t lastReadCallSite_;
Expand Down Expand Up @@ -3259,6 +3260,7 @@ class BaseCompiler final : public BaseCompilerInterface {

public:
BaseCompiler(const ModuleEnvironment& moduleEnv,
const CompilerEnvironment& compilerEnv,
const FuncCompileInput& input, const ValTypeVector& locals,
const MachineState& trapExitLayout,
size_t trapExitLayoutNumWords, Decoder& decoder,
Expand Down Expand Up @@ -4169,7 +4171,7 @@ class BaseCompiler final : public BaseCompilerInterface {
const ExitStubMapVector& extras,
uint32_t assemblerOffset) {
auto debugFrame =
moduleEnv_.debugEnabled() ? HasDebugFrame::Yes : HasDebugFrame::No;
compilerEnv_.debugEnabled() ? HasDebugFrame::Yes : HasDebugFrame::No;
return stackMapGenerator_.createStackMap(who, extras, assemblerOffset,
debugFrame, stk_);
}
Expand Down Expand Up @@ -5277,10 +5279,11 @@ class BaseCompiler final : public BaseCompilerInterface {
}
}

GenerateFunctionPrologue(
masm, moduleEnv_.funcTypes[func_.index]->id,
moduleEnv_.mode() == CompileMode::Tier1 ? Some(func_.index) : Nothing(),
&offsets_);
GenerateFunctionPrologue(masm, moduleEnv_.funcTypes[func_.index]->id,
compilerEnv_.mode() == CompileMode::Tier1
? Some(func_.index)
: Nothing(),
&offsets_);

// GenerateFunctionPrologue pushes exactly one wasm::Frame's worth of
// stuff, and none of the values are GC pointers. Hence:
Expand All @@ -5292,7 +5295,7 @@ class BaseCompiler final : public BaseCompilerInterface {
// Initialize DebugFrame fields before the stack overflow trap so that
// we have the invariant that all observable Frames in a debugEnabled
// Module have valid DebugFrames.
if (moduleEnv_.debugEnabled()) {
if (compilerEnv_.debugEnabled()) {
#ifdef JS_CODEGEN_ARM64
static_assert(DebugFrame::offsetOfFrame() % WasmStackAlignment == 0,
"aligned");
Expand Down Expand Up @@ -5359,7 +5362,7 @@ class BaseCompiler final : public BaseCompilerInterface {
}
// If we're in a debug frame, copy the stack result pointer arg
// to a well-known place.
if (moduleEnv_.debugEnabled()) {
if (compilerEnv_.debugEnabled()) {
Register target = ABINonArgReturnReg0;
fr.loadIncomingStackResultAreaPtr(RegPtr(target));
size_t debugFrameOffset =
Expand Down Expand Up @@ -5410,7 +5413,7 @@ class BaseCompiler final : public BaseCompilerInterface {
fr.zeroLocals(&ra);
fr.storeTlsPtr(WasmTlsReg);

if (moduleEnv_.debugEnabled()) {
if (compilerEnv_.debugEnabled()) {
insertBreakablePoint(CallSiteDesc::EnterFrame);
if (!createStackMap("debug: breakable point")) {
return false;
Expand Down Expand Up @@ -5438,7 +5441,7 @@ class BaseCompiler final : public BaseCompilerInterface {
}

void saveRegisterReturnValues(const ResultType& resultType) {
MOZ_ASSERT(moduleEnv_.debugEnabled());
MOZ_ASSERT(compilerEnv_.debugEnabled());
size_t debugFrameOffset = masm.framePushed() - DebugFrame::offsetOfFrame();
size_t registerResultIdx = 0;
for (ABIResultIter i(resultType); !i.done(); i.next()) {
Expand Down Expand Up @@ -5491,7 +5494,7 @@ class BaseCompiler final : public BaseCompilerInterface {
}

void restoreRegisterReturnValues(const ResultType& resultType) {
MOZ_ASSERT(moduleEnv_.debugEnabled());
MOZ_ASSERT(compilerEnv_.debugEnabled());
size_t debugFrameOffset = masm.framePushed() - DebugFrame::offsetOfFrame();
size_t registerResultIdx = 0;
for (ABIResultIter i(resultType); !i.done(); i.next()) {
Expand Down Expand Up @@ -5557,7 +5560,7 @@ class BaseCompiler final : public BaseCompilerInterface {

popStackReturnValues(resultType);

if (moduleEnv_.debugEnabled()) {
if (compilerEnv_.debugEnabled()) {
// Store and reload the return value from DebugFrame::return so that
// it can be clobbered, and/or modified by the debug trap.
saveRegisterReturnValues(resultType);
Expand Down Expand Up @@ -10944,7 +10947,7 @@ bool BaseCompiler::emitSetGlobal() {
//
// Finally, when the debugger allows locals to be mutated we must disable BCE
// for references via a local, by returning immediately from bceCheckLocal if
// moduleEnv_.debugEnabled() is true.
// compilerEnv_.debugEnabled() is true.
//
//
// Alignment check elimination.
Expand Down Expand Up @@ -14043,12 +14046,12 @@ bool BaseCompiler::emitBody() {
OpBytes op;
CHECK(iter_.readOp(&op));

// When moduleEnv_.debugEnabled(), every operator has breakpoint site but
// When compilerEnv_.debugEnabled(), every operator has breakpoint site but
// Op::End.
if (moduleEnv_.debugEnabled() && op.b0 != (uint16_t)Op::End) {
if (compilerEnv_.debugEnabled() && op.b0 != (uint16_t)Op::End) {
// TODO sync only registers that can be clobbered by the exit
// prologue/epilogue or disable these registers for use in
// baseline compiler when moduleEnv_.debugEnabled() is set.
// baseline compiler when compilerEnv_.debugEnabled() is set.
sync();

insertBreakablePoint(CallSiteDesc::Breakpoint);
Expand Down Expand Up @@ -15465,13 +15468,15 @@ bool BaseCompiler::emitFunction() {
}

BaseCompiler::BaseCompiler(const ModuleEnvironment& moduleEnv,
const CompilerEnvironment& compilerEnv,
const FuncCompileInput& func,
const ValTypeVector& locals,
const MachineState& trapExitLayout,
size_t trapExitLayoutNumWords, Decoder& decoder,
StkVector& stkSource, TempAllocator* alloc,
MacroAssembler* masm, StackMaps* stackMaps)
: moduleEnv_(moduleEnv),
compilerEnv_(compilerEnv),
iter_(moduleEnv, decoder),
func_(func),
lastReadCallSite_(0),
Expand Down Expand Up @@ -15520,7 +15525,8 @@ bool BaseCompiler::init() {
}

ArgTypeVector args(funcType());
if (!fr.setupLocals(locals_, args, moduleEnv_.debugEnabled(), &localInfo_)) {
if (!fr.setupLocals(locals_, args, compilerEnv_.debugEnabled(),
&localInfo_)) {
return false;
}

Expand Down Expand Up @@ -15564,11 +15570,12 @@ bool js::wasm::BaselinePlatformSupport() {
}

bool js::wasm::BaselineCompileFunctions(const ModuleEnvironment& moduleEnv,
const CompilerEnvironment& compilerEnv,
LifoAlloc& lifo,
const FuncCompileInputVector& inputs,
CompiledCode* code,
UniqueChars* error) {
MOZ_ASSERT(moduleEnv.tier() == Tier::Baseline);
MOZ_ASSERT(compilerEnv.tier() == Tier::Baseline);
MOZ_ASSERT(moduleEnv.kind == ModuleKind::Wasm);

// The MacroAssembler will sometimes access the jitContext.
Expand Down Expand Up @@ -15612,7 +15619,7 @@ bool js::wasm::BaselineCompileFunctions(const ModuleEnvironment& moduleEnv,

// One-pass baseline compilation.

BaseCompiler f(moduleEnv, func, locals, trapExitLayout,
BaseCompiler f(moduleEnv, compilerEnv, func, locals, trapExitLayout,
trapExitLayoutNumWords, d, stk, &alloc, &masm,
&code->stackMaps);
if (!f.init()) {
Expand Down
9 changes: 4 additions & 5 deletions js/src/wasm/WasmBaselineCompile.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,10 @@ namespace wasm {
MOZ_MUST_USE bool BaselinePlatformSupport();

// Generate adequate code quickly.
MOZ_MUST_USE bool BaselineCompileFunctions(const ModuleEnvironment& moduleEnv,
LifoAlloc& lifo,
const FuncCompileInputVector& inputs,
CompiledCode* code,
UniqueChars* error);
MOZ_MUST_USE bool BaselineCompileFunctions(
const ModuleEnvironment& moduleEnv, const CompilerEnvironment& compilerEnv,
LifoAlloc& lifo, const FuncCompileInputVector& inputs, CompiledCode* code,
UniqueChars* error);

class BaseLocalIter {
private:
Expand Down
22 changes: 12 additions & 10 deletions js/src/wasm/WasmCompile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -576,13 +576,14 @@ SharedModule wasm::CompileBuffer(const CompileArgs& args,
JSTelemetrySender telemetrySender) {
Decoder d(bytecode.bytes, 0, error, warnings);

CompilerEnvironment compilerEnv(args);
ModuleEnvironment moduleEnv(&compilerEnv, args.features);
ModuleEnvironment moduleEnv(args.features);
if (!DecodeModuleEnvironment(d, &moduleEnv)) {
return nullptr;
}
CompilerEnvironment compilerEnv(args);
compilerEnv.computeParameters(d);

ModuleGenerator mg(args, &moduleEnv, nullptr, error);
ModuleGenerator mg(args, &moduleEnv, &compilerEnv, nullptr, error);
if (!mg.init(nullptr, telemetrySender)) {
return nullptr;
}
Expand All @@ -608,15 +609,15 @@ void wasm::CompileTier2(const CompileArgs& args, const Bytes& bytecode,
? OptimizedBackend::Cranelift
: OptimizedBackend::Ion;

CompilerEnvironment compilerEnv(CompileMode::Tier2, Tier::Optimized,
optimizedBackend, DebugEnabled::False);

ModuleEnvironment moduleEnv(&compilerEnv, args.features);
ModuleEnvironment moduleEnv(args.features);
if (!DecodeModuleEnvironment(d, &moduleEnv)) {
return;
}
CompilerEnvironment compilerEnv(CompileMode::Tier2, Tier::Optimized,
optimizedBackend, DebugEnabled::False);
compilerEnv.computeParameters(d);

ModuleGenerator mg(args, &moduleEnv, cancelled, &error);
ModuleGenerator mg(args, &moduleEnv, &compilerEnv, cancelled, &error);
if (!mg.init(nullptr, telemetrySender)) {
return;
}
Expand Down Expand Up @@ -719,14 +720,15 @@ SharedModule wasm::CompileStreaming(
const Atomic<bool>& cancelled, UniqueChars* error,
UniqueCharsVector* warnings, JSTelemetrySender telemetrySender) {
CompilerEnvironment compilerEnv(args);
ModuleEnvironment moduleEnv(&compilerEnv, args.features);
ModuleEnvironment moduleEnv(args.features);

{
Decoder d(envBytes, 0, error, warnings);

if (!DecodeModuleEnvironment(d, &moduleEnv)) {
return nullptr;
}
compilerEnv.computeParameters(d);

if (!moduleEnv.codeSection) {
d.fail("unknown section before code section");
Expand All @@ -737,7 +739,7 @@ SharedModule wasm::CompileStreaming(
MOZ_RELEASE_ASSERT(d.done());
}

ModuleGenerator mg(args, &moduleEnv, &cancelled, error);
ModuleGenerator mg(args, &moduleEnv, &compilerEnv, &cancelled, error);
if (!mg.init(nullptr, telemetrySender)) {
return nullptr;
}
Expand Down
6 changes: 4 additions & 2 deletions js/src/wasm/WasmCraneliftCompile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -433,13 +433,15 @@ const GlobalDesc* env_global(const CraneliftModuleEnvironment* wrapper,
}

bool wasm::CraneliftCompileFunctions(const ModuleEnvironment& moduleEnv,
const CompilerEnvironment& compilerEnv,
LifoAlloc& lifo,
const FuncCompileInputVector& inputs,
CompiledCode* code, UniqueChars* error) {
MOZ_RELEASE_ASSERT(CraneliftPlatformSupport());

MOZ_ASSERT(moduleEnv.tier() == Tier::Optimized);
MOZ_ASSERT(moduleEnv.optimizedBackend() == OptimizedBackend::Cranelift);
MOZ_ASSERT(compilerEnv.tier() == Tier::Optimized);
MOZ_ASSERT(compilerEnv.debug() == DebugEnabled::False);
MOZ_ASSERT(compilerEnv.optimizedBackend() == OptimizedBackend::Cranelift);
MOZ_ASSERT(!moduleEnv.isAsmJS());

TempAllocator alloc(&lifo);
Expand Down
8 changes: 4 additions & 4 deletions js/src/wasm/WasmCraneliftCompile.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,17 @@ MOZ_MUST_USE bool CraneliftPlatformSupport();

// Generates code with Cranelift.
MOZ_MUST_USE bool CraneliftCompileFunctions(
const ModuleEnvironment& moduleEnv, LifoAlloc& lifo,
const FuncCompileInputVector& inputs, CompiledCode* code,
const ModuleEnvironment& moduleEnv, const CompilerEnvironment& compilerEnv,
LifoAlloc& lifo, const FuncCompileInputVector& inputs, CompiledCode* code,
UniqueChars* error);

void CraneliftFreeReusableData(void* data);
#else
MOZ_MUST_USE inline bool CraneliftPlatformSupport() { return false; }

MOZ_MUST_USE inline bool CraneliftCompileFunctions(
const ModuleEnvironment& moduleEnv, LifoAlloc& lifo,
const FuncCompileInputVector& inputs, CompiledCode* code,
const ModuleEnvironment& moduleEnv, const CompilerEnvironment& compilerEnv,
LifoAlloc& lifo, const FuncCompileInputVector& inputs, CompiledCode* code,
UniqueChars* error) {
MOZ_CRASH("Should not happen");
}
Expand Down
Loading

0 comments on commit fb950b7

Please sign in to comment.