[liftoff] Fix missing stack move
The {operator==} on {VarState} did not check the spill offset, so when
merging stack states, we forgot to move stack values if both source and
destination were stack slots, but at different offsets.
This CL fixes this by removing the {operator==}, because the semantics
(and use) are not clear, and it's only used in one place anyway.
The equality check was mostly redundant, so inlining it also makes the
code smaller and faster.
R=ahaas@chromium.org
Bug: v8:10702
Change-Id: I6c8b2cfd1002274175c9a17d305692e4631fd7dc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2304574
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68916}
diff --git a/src/wasm/baseline/liftoff-assembler.cc b/src/wasm/baseline/liftoff-assembler.cc
index 4e9e8c2..24c2e0a 100644
--- a/src/wasm/baseline/liftoff-assembler.cc
+++ b/src/wasm/baseline/liftoff-assembler.cc
@@ -84,16 +84,20 @@
V8_INLINE void TransferStackSlot(const VarState& dst, const VarState& src) {
DCHECK_EQ(dst.type(), src.type());
- if (dst == src) return;
if (dst.is_reg()) {
LoadIntoRegister(dst.reg(), src, src.offset());
return;
}
+ if (dst.is_const()) {
+ DCHECK_EQ(dst.i32_const(), src.i32_const());
+ return;
+ }
DCHECK(dst.is_stack());
switch (src.loc()) {
case VarState::kStack:
- DCHECK_NE(src.offset(), dst.offset());
- asm_->MoveStackValue(dst.offset(), src.offset(), src.type());
+ if (src.offset() != dst.offset()) {
+ asm_->MoveStackValue(dst.offset(), src.offset(), src.type());
+ }
break;
case VarState::kRegister:
asm_->Spill(dst.offset(), src.reg(), src.type());
diff --git a/src/wasm/baseline/liftoff-assembler.h b/src/wasm/baseline/liftoff-assembler.h
index ecfa8dd..94991fe 100644
--- a/src/wasm/baseline/liftoff-assembler.h
+++ b/src/wasm/baseline/liftoff-assembler.h
@@ -56,20 +56,6 @@
DCHECK(type_ == kWasmI32 || type_ == kWasmI64);
}
- bool operator==(const VarState& other) const {
- if (loc_ != other.loc_) return false;
- if (type_ != other.type_) return false;
- switch (loc_) {
- case kStack:
- return true;
- case kRegister:
- return reg_ == other.reg_;
- case kIntConst:
- return i32_const_ == other.i32_const_;
- }
- UNREACHABLE();
- }
-
bool is_stack() const { return loc_ == kStack; }
bool is_gp_reg() const { return loc_ == kRegister && reg_.is_gp(); }
bool is_fp_reg() const { return loc_ == kRegister && reg_.is_fp(); }
diff --git a/test/mjsunit/regress/wasm/regress-10702.js b/test/mjsunit/regress/wasm/regress-10702.js
new file mode 100644
index 0000000..d1ec949
--- /dev/null
+++ b/test/mjsunit/regress/wasm/regress-10702.js
@@ -0,0 +1,40 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+const builder = new WasmModuleBuilder();
+builder.addGlobal(kWasmI32, 1).init = 35;
+builder.addType(makeSig([], [kWasmI32]));
+builder.addType(makeSig([kWasmI32, kWasmI32], [kWasmI32]));
+// Generate function 1 (out of 3).
+builder.addFunction(undefined, 0 /* sig */).addBody([
+ // signature: i_v
+ // body:
+ kExprI32Const, 1, // i32.const
+]);
+// Generate function 2 (out of 3).
+builder.addFunction(undefined, 0 /* sig */).addBody([
+ // signature: i_v
+ // body:
+ kExprI32Const, 0, // i32.const
+]);
+// Generate function 3 (out of 3).
+builder.addFunction(undefined, 1 /* sig */).addBody([
+ // signature: i_ii
+ // body:
+ kExprBlock, kWasmI32, // block @1 i32
+ kExprF64Const, 0, 0, 0, 0, 0, 0, 0, 0, // f64.const
+ kExprI32SConvertF64, // i32.trunc_f64_s
+ kExprCallFunction, 1, // call function #1: i_v
+ kExprBrIf, 0, // br_if depth=0
+ kExprGlobalGet, 0, // global.get
+ kExprCallFunction, 0, // call function #0: i_v
+ kExprBrIf, 0, // br_if depth=0
+ kExprI32ShrS, // i32.shr_s
+ kExprEnd, // end @24
+]);
+builder.addExport('f', 2);
+const instance = builder.instantiate();
+assertEquals(35, instance.exports.f(0, 0));
