Skip to content

Commit

Permalink
[wasm] Do not call CheckForException on every node
Browse files Browse the repository at this point in the history
Calling CheckForException through the BUILD macro for almost every
invocation of WasmGraphBuilder from graph-builder-interface created
multiple issues:
- It generated an invalid graph with GetExceptionValues, resulting in
  us having to hack CallBuiltin().
- It will generate multiple problems with wasm-gc operations.
- It needlessly slowed down compilation.

Changes:
- Remove BUILD macro, call CheckForException directly only for function
  calls and throws.
- Split CheckForException into a fast inline path and a slow non-inline
  one.
- Move BUILD's DCHECK into CHECK_INTERFACE.
- Add an early exit to Decode().
- Since we do not need the hack anymore, implement CallBuiltin properly.

Change-Id: I38e3c73e0af80894584ce37e2f3bfc90111ac54f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2717304
Commit-Queue: Manos Koukoutos <[email protected]>
Reviewed-by: Thibaud Michaud <[email protected]>
Reviewed-by: Clemens Backes <[email protected]>
Cr-Commit-Position: refs/heads/master@{#73036}
  • Loading branch information
manoskouk authored and Commit Bot committed Feb 25, 2021
1 parent 43648c5 commit 4789213
Show file tree
Hide file tree
Showing 3 changed files with 146 additions and 136 deletions.
10 changes: 1 addition & 9 deletions src/compiler/wasm-compiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -204,18 +204,10 @@ class WasmGraphAssembler : public GraphAssembler {

template <typename... Args>
Node* CallBuiltin(Builtins::Name name, Args*... args) {
// We would like to use gasm_->Call() to implement this method,
// but this doesn't work currently when we try to call it from functions
// which set IfSuccess/IfFailure control paths (e.g. within Throw()).
// TODO(manoskouk): Maybe clean this up at some point and unite with
// CallRuntimeStub?
auto* call_descriptor = GetBuiltinCallDescriptor(
name, temp_zone(), StubCallMode::kCallBuiltinPointer);
Node* call_target = GetBuiltinPointerTarget(name);
Node* call = graph()->NewNode(mcgraph()->common()->Call(call_descriptor),
call_target, args..., effect(), control());
InitializeEffectControl(call, control());
return call;
return Call(call_descriptor, call_target, args...);
}

void EnsureEnd() {
Expand Down
10 changes: 8 additions & 2 deletions src/wasm/function-body-decoder-impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -2176,7 +2176,11 @@ MemoryAccessImmediate<validate>::MemoryAccessImmediate(
: MemoryAccessImmediate(decoder, pc, max_alignment,
decoder->module_->is_memory64) {}

#define CALL_INTERFACE(name, ...) interface_.name(this, ##__VA_ARGS__)
#define CALL_INTERFACE(name, ...) \
do { \
DCHECK(this->ok()); \
interface_.name(this, ##__VA_ARGS__); \
} while (false)
#define CALL_INTERFACE_IF_REACHABLE(name, ...) \
do { \
DCHECK(!control_.empty()); \
Expand Down Expand Up @@ -2227,6 +2231,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
uint32_t params_count = static_cast<uint32_t>(this->num_locals());
uint32_t locals_length;
this->DecodeLocals(this->pc(), &locals_length, params_count);
if (this->failed()) return TraceFailed();
this->consume_bytes(locals_length);
for (uint32_t index = params_count; index < this->num_locals(); index++) {
if (!VALIDATE(this->local_type(index).is_defaultable())) {
Expand Down Expand Up @@ -4770,7 +4775,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
this->end_ = this->pc_; // Terminate decoding loop.
this->current_code_reachable_ = false;
TRACE(" !%s\n", this->error_.message().c_str());
CALL_INTERFACE(OnFirstError);
// Cannot use CALL_INTERFACE here because it checks if this->ok().
interface_.OnFirstError(this);
}

int BuildSimplePrototypeOperator(WasmOpcode opcode) {
Expand Down
Loading

0 comments on commit 4789213

Please sign in to comment.