[wasm] Modify decoder's value stack before interface calls [v8/v8 : main]
Clemens Backes (Gerrit)
Attention is currently required from: Jakob Kummerow.
1 comment:
Patchset:
Finally, here it is! PTAL.
To view, visit change 4525900. To unsubscribe, or for help writing mail filters, visit settings.
Jakob Kummerow (Gerrit)
Attention is currently required from: Clemens Backes.
Patch set 3:Code-Review +1
12 comments:
Patchset:
LGTM with comments.
File src/wasm/function-body-decoder-impl.h:
// TODO(jkummerow): Consider moving the stack manipulation after the
// INTERFACE call for consistency.This is now outdated, just drop it.
// Drop {value_on_branch} from the stack.
Drop(1);Suggestion: if we had a variant of `void Drop(const Value& /* unused */)` that takes a `const Value*`, we could write `Drop(value_on_branch);` here and wouldn't need the comment.
Patch Set #3, Line 3629:
ReturnVector returns = CreateReturnValues(imm.sig);Is this intentionally not using `PushReturns` (like CallFunction/CallIndirect above)?
Patch Set #3, Line 4269:
CreateReturnValuesIs this intentionally not using `PushReturns`?
Patch Set #3, Line 4332:
Push(rtt);I'm fine with postponing all rtt handling until later (the CL is big enough), but I'd think it should be possible to simply not push/drop the `rtt`. If the `args` are already not on the stack during the interface call, there's no reason why the `rtt` should be.
// Don't bother pushing the rtt, as we'd drop it again immediately
// anyway.Here you see an example of an `rtt` that we simply keep on the C++ stack without ever pushing it onto the value stack.
// Attention: contrary to most other instructions, we modify the
// stack before calling the interface function. This makes it
// significantly more convenient to pass around the values that
// will be on the stack when the branch is taken.
// TODO(jkummerow): Reconsider this choice.obsolete
Push(CreateValue(ValueType::Ref(imm.index)));
// The {value_on_branch} parameter we pass to the interface must
// be pointer-identical to the object on the stack.
Value* value_on_branch = stack_value(1);I think this can now be just:
```
Value* value_on_branch = Push(ValueType::Ref(imm.index));
```Patch Set #3, Line 5251:
CreateValueI think this could `Push` directly. Or just don't bother; these instructions are all deprecated anyway and will be deleted before we ship WasmGC unflagged.
Value result_on_branch = CreateValue(ValueType::Ref(heap_type));
Push(result_on_branch);I think this can be:
```
Value* value_on_branch = Push(ValueType::Ref(heap_type));
```
and then you can drop lines 5338-5341 (and update 5356).Or just don't bother, because these instructions are also deprecated :-)
I don't think this is correct: we generally want the "value on branch" to be on the stack for possibly-branching instructions, the "value on fallthrough" is pushed afterwards. See e.g. lines 5356/5357.
These instructions, too, are deprecated, and it seems that we have already migrated our test coverage to use their replacements (based on an assumption that we wouldn't refactor this code before deleting it), so the green try jobs don't indicate that this change is OK.
To view, visit change 4525900. To unsubscribe, or for help writing mail filters, visit settings.
Clemens Backes (Gerrit)
11 comments:
Patchset:
Addressed comments; won't land just before the weekend though, so this has to wait until Tuesday.
File src/wasm/function-body-decoder-impl.h:
// TODO(jkummerow): Consider moving the stack manipulation after the
// INTERFACE call for consistency.This is now outdated, just drop it.
Done
// Drop {value_on_branch} from the stack.
Drop(1);
Suggestion: if we had a variant of `void Drop(const Value& /* unused */)` that takes a `const Value* […]
Ack; used `Drop(*value_on_branch)`.
Patch Set #3, Line 3629:
ReturnVector returns = CreateReturnValues(imm.sig);Is this intentionally not using `PushReturns` (like CallFunction/CallIndirect above)?
Nope, oversight. Also fixed another last use of `CreateReturnValues` and dropped that method.
Patch Set #3, Line 4269:
CreateReturnValuesIs this intentionally not using `PushReturns`?
Done
Patch Set #3, Line 4332:
Push(rtt);
I'm fine with postponing all rtt handling until later (the CL is big enough), but I'd think it shoul […]
I already have a commit lined up to fix the rtt stuff. I'd suggest postponing until that is ready.
// Attention: contrary to most other instructions, we modify the
// stack before calling the interface function. This makes it
// significantly more convenient to pass around the values that
// will be on the stack when the branch is taken.
// TODO(jkummerow): Reconsider this choice.obsolete
Done
Push(CreateValue(ValueType::Ref(imm.index)));
// The {value_on_branch} parameter we pass to the interface must
// be pointer-identical to the object on the stack.
Value* value_on_branch = stack_value(1);
I think this can now be just: […]
Done
Patch Set #3, Line 5251:
CreateValue
I think this could `Push` directly. […]
Ack, changed this to `Push` directly (also below).
Value result_on_branch = CreateValue(ValueType::Ref(heap_type));
Push(result_on_branch);
I think this can be: […]
Done
I don't think this is correct: we generally want the "value on branch" to be on the stack for possib […]
Ack, I undid this change. We still have the old methods, so no need to migrate those deprecated opcodes.
To view, visit change 4525900. To unsubscribe, or for help writing mail filters, visit settings.
Clemens Backes (Gerrit)
Attention is currently required from: Clemens Backes.
Patch set 4:Commit-Queue +2
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/wasm/function-body-decoder-impl.h
Insertions: 27, Deletions: 51.
@@ -3012,8 +3012,6 @@
}
FallThrough();
c->kind = kControlTryCatch;
- // TODO(jkummerow): Consider moving the stack manipulation after the
- // INTERFACE call for consistency.
stack_.shrink_to(c->stack_depth);
c->reachability = control_at(1)->innerReachability();
RollbackLocalsInitialization(c);
@@ -3157,8 +3155,7 @@
PopTypeError(0, ref_object, "object reference");
return 0;
}
- // Drop {value_on_branch} from the stack.
- Drop(1);
+ Drop(*value_on_branch);
return 1 + imm.length;
}
@@ -3626,10 +3623,9 @@
if (!this->Validate(this->pc_ + 1, imm)) return 0;
Value func_ref = Pop(ValueType::RefNull(imm.index));
PoppedArgVector args = PopArgs(imm.sig);
- ReturnVector returns = CreateReturnValues(imm.sig);
+ Value* returns = PushReturns(imm.sig);
CALL_INTERFACE_IF_OK_AND_REACHABLE(CallRef, func_ref, imm.sig, imm.index,
- args.begin(), returns.begin());
- PushReturns(returns);
+ args.begin(), returns);
return 1 + imm.length;
}
@@ -4266,10 +4262,9 @@
CALL_INTERFACE_IF_OK_AND_REACHABLE(SimdOp, opcode,
base::VectorOf(args), nullptr);
} else {
- ReturnVector results = CreateReturnValues(sig);
- CALL_INTERFACE_IF_OK_AND_REACHABLE(
- SimdOp, opcode, base::VectorOf(args), results.begin());
- PushReturns(results);
+ Value* results = PushReturns(sig);
+ CALL_INTERFACE_IF_OK_AND_REACHABLE(SimdOp, opcode,
+ base::VectorOf(args), results);
}
return opcode_length;
}
@@ -5124,16 +5119,8 @@
"br_on_cast must target a branch of arity at least 1");
return 0;
}
- // Attention: contrary to most other instructions, we modify the
- // stack before calling the interface function. This makes it
- // significantly more convenient to pass around the values that
- // will be on the stack when the branch is taken.
- // TODO(jkummerow): Reconsider this choice.
Drop(obj);
- Push(CreateValue(ValueType::Ref(imm.index)));
- // The {value_on_branch} parameter we pass to the interface must
- // be pointer-identical to the object on the stack.
- Value* value_on_branch = stack_value(1);
+ Value* value_on_branch = Push(ValueType::Ref(imm.index));
if (!VALIDATE(TypeCheckBranch<true>(c))) return 0;
if (V8_LIKELY(current_code_reachable_and_ok_)) {
// This logic ensures that code generation can assume that functions
@@ -5248,27 +5235,26 @@
NON_CONST_ONLY \
Value arg = Pop(kWasmAnyRef); \
if (!VALIDATE(this->ok())) return 0; \
- Value result = CreateValue(kWasmI32); \
+ Value* result = Push(kWasmI32); \
if (V8_LIKELY(current_code_reachable_and_ok_)) { \
if (IsHeapSubtypeOf(arg.type.heap_type(), HeapType(HeapType::k##h_type), \
this->module_)) { \
if (arg.type.is_nullable()) { \
/* We abuse ref.as_non_null, which isn't otherwise used as a unary \
* operator, as a sentinel for the negation of ref.is_null. */ \
- CALL_INTERFACE(UnOp, kExprRefAsNonNull, arg, &result); \
+ CALL_INTERFACE(UnOp, kExprRefAsNonNull, arg, result); \
} else { \
CALL_INTERFACE(Drop); \
- CALL_INTERFACE(I32Const, &result, 1); \
+ CALL_INTERFACE(I32Const, result, 1); \
} \
} else if (!IsHeapSubtypeOf(HeapType(HeapType::k##h_type), \
arg.type.heap_type(), this->module_)) { \
CALL_INTERFACE(Drop); \
- CALL_INTERFACE(I32Const, &result, 0); \
+ CALL_INTERFACE(I32Const, result, 0); \
} else { \
- CALL_INTERFACE(RefIs##h_type, arg, &result); \
+ CALL_INTERFACE(RefIs##h_type, arg, result); \
} \
} \
- Push(result); \
return opcode_length; \
}
ABSTRACT_TYPE_CHECK(Struct)
@@ -5282,14 +5268,14 @@
Value arg = Pop(kWasmAnyRef); \
ValueType non_nullable_abstract_type = \
ValueType::Ref(HeapType::k##h_type); \
- Value result = CreateValue(non_nullable_abstract_type); \
+ Value* result = Push(non_nullable_abstract_type); \
if (V8_LIKELY(current_code_reachable_and_ok_)) { \
if (IsHeapSubtypeOf(arg.type.heap_type(), HeapType(HeapType::k##h_type), \
this->module_)) { \
if (arg.type.is_nullable()) { \
- CALL_INTERFACE(RefAsNonNull, arg, &result); \
+ CALL_INTERFACE(RefAsNonNull, arg, result); \
} else { \
- CALL_INTERFACE(Forward, arg, &result); \
+ CALL_INTERFACE(Forward, arg, result); \
} \
} else if (!IsHeapSubtypeOf(HeapType(HeapType::k##h_type), \
arg.type.heap_type(), this->module_)) { \
@@ -5298,10 +5284,9 @@
/* to the spec it technically is. Set it to spec-only reachable. */ \
SetSucceedingCodeDynamicallyUnreachable(); \
} else { \
- CALL_INTERFACE(RefAs##h_type, arg, &result); \
+ CALL_INTERFACE(RefAs##h_type, arg, result); \
} \
} \
- Push(result); \
return opcode_length; \
}
ABSTRACT_TYPE_CAST(Struct)
@@ -5332,13 +5317,8 @@
opcode == kExprBrOnStruct ? HeapType::kStruct
: opcode == kExprBrOnArray ? HeapType::kArray
: HeapType::kI31;
- Value result_on_branch = CreateValue(ValueType::Ref(heap_type));
- Push(result_on_branch);
+ Value* value_on_branch = Push(ValueType::Ref(heap_type));
if (!VALIDATE(TypeCheckBranch<true>(c))) return 0;
- // The {value_on_branch} parameter we pass to the interface must be
- // pointer-identical to the object on the stack, so we can't reuse
- // {result_on_branch} which was passed-by-value to {Push}.
- Value* value_on_branch = stack_value(1);
if (V8_LIKELY(current_code_reachable_and_ok_)) {
bool null_succeeds = false;
if (opcode == kExprBrOnStruct) {
@@ -5353,8 +5333,9 @@
}
c->br_merge()->reached = true;
}
- Drop(result_on_branch);
- Push(obj); // Restore stack state on fallthrough.
+ // Restore stack state on fallthrough.
+ Drop(*value_on_branch);
+ Push(obj);
return opcode_length + branch_depth.length;
}
case kExprBrOnNonStruct:
@@ -5376,27 +5357,29 @@
}
if (!VALIDATE(TypeCheckBranch<true>(c))) return 0;
- Value obj = Pop(kWasmAnyRef);
+ Value obj = Peek(kWasmAnyRef);
HeapType::Representation heap_type =
opcode == kExprBrOnNonStruct ? HeapType::kStruct
: opcode == kExprBrOnNonArray ? HeapType::kArray
: HeapType::kI31;
- Value* value_on_fallthrough = Push(ValueType::Ref(heap_type));
+ Value value_on_fallthrough = CreateValue(ValueType::Ref(heap_type));
if (V8_LIKELY(current_code_reachable_and_ok_)) {
bool null_succeeds = false;
if (opcode == kExprBrOnNonStruct) {
- CALL_INTERFACE(BrOnNonStruct, obj, value_on_fallthrough,
+ CALL_INTERFACE(BrOnNonStruct, obj, &value_on_fallthrough,
branch_depth.depth, null_succeeds);
} else if (opcode == kExprBrOnNonArray) {
- CALL_INTERFACE(BrOnNonArray, obj, value_on_fallthrough,
+ CALL_INTERFACE(BrOnNonArray, obj, &value_on_fallthrough,
branch_depth.depth, null_succeeds);
} else {
- CALL_INTERFACE(BrOnNonI31, obj, value_on_fallthrough,
+ CALL_INTERFACE(BrOnNonI31, obj, &value_on_fallthrough,
branch_depth.depth, null_succeeds);
}
c->br_merge()->reached = true;
}
+ Drop(obj);
+ Push(value_on_fallthrough);
return opcode_length + branch_depth.length;
}
case kExprExternInternalize: {
@@ -6285,13 +6268,6 @@
DCHECK_EQ(c->stack_depth + merge->arity, stack_.size());
}
- V8_INLINE ReturnVector CreateReturnValues(const FunctionSig* sig) {
- size_t return_count = sig->return_count();
- ReturnVector values(return_count);
- std::transform(sig->returns().begin(), sig->returns().end(), values.begin(),
- [this](ValueType type) { return CreateValue(type); });
- return values;
- }
V8_INLINE void PushReturns(ReturnVector values) {
stack_.EnsureMoreCapacity(static_cast<int>(values.size()), this->zone_);
for (Value& value : values) Push(value);
```
[wasm] Modify decoder's value stack before interface calls
This switches the majority of opcodes back from a sequence of
- peek arguments
- create returns
- call interface
- drop arguments
- push returns
to
- pop arguments
- push returns
- call interface
This is both simpler (no dependency between the number of peeked and
dropped values), and more efficient because the return values can be
stored on the value stack directly instead of being copied after the
interface call.
For now we need to copy the popped arguments into another SmallVector
living on the physical stack frame, but for Liftoff this can be avoided
because Liftoff never uses the popped arguments. This will be optimized
in a follow-up CL.
Some uses of the previous sequence remain, mostly WasmGC instructions
dealing with an implicit rtt object. Converting these is more difficult
and will be done after the rtt handling has been improved (by not
pushing the rtt to the decoder's value stack).
R=jkum...@chromium.org
Bug: v8:13976
Change-Id: I872841076004afe25268897b14bd1d3db076cd6a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4525900
Commit-Queue: Clemens Backes <clem...@chromium.org>
Reviewed-by: Jakob Kummerow <jkum...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#87692}
---
M src/wasm/baseline/liftoff-compiler.cc
M src/wasm/function-body-decoder-impl.h
M src/wasm/graph-builder-interface.cc
M test/unittests/wasm/function-body-decoder-unittest.cc
M test/unittests/wasm/module-decoder-unittest.cc
5 files changed, 441 insertions(+), 625 deletions(-)