Skip to content

Commit

Permalink
[wasm-simd][liftoff][arm64] Check offset fits in str immediate
Browse files Browse the repository at this point in the history
When filling stack slots, the start offset can be too large to fit into
the immediate of a str instruction (which is used to handle remainders
after stp). For example, a function with 32 i64 params will require 256
bytes reserved for the params, so the offset starts at 256 + 16
(instance) = 272. This does not fit into a int9, so we hit an
UNREACHABLE case when emitting str.

The fix here checks that start can fit in an unscaled immediate, and if
it doesn't fallback to the general case. We could use the Str
from macro-asesmbler, but that uses another instruction, so we are not
saving anything.

A check for IsImmLSUnscaled(-start-12) is sufficient because 12 is the
largest possible value for remainder. So if -start-12 fits, everything
else will fit.

Bug: v8:10645
Change-Id: I1c415499ada3a807d5f3889f091150bfefdf471d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2267369
Commit-Queue: Zhi An Ng <[email protected]>
Reviewed-by: Clemens Backes <[email protected]>
Cr-Commit-Position: refs/heads/master@{#68594}
  • Loading branch information
ngzhian authored and Commit Bot committed Jun 29, 2020
1 parent 32b685f commit c92e74f
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 1 deletion.
10 changes: 9 additions & 1 deletion src/wasm/baseline/arm64/liftoff-assembler-arm64.h
Original file line number Diff line number Diff line change
Expand Up @@ -774,13 +774,21 @@ void LiftoffAssembler::FillI64Half(Register, int offset, RegPairHalf) {
}

void LiftoffAssembler::FillStackSlotsWithZero(int start, int size) {
// Zero 'size' bytes *below* start, byte at offset 'start' is untouched.
DCHECK_LE(0, start);
DCHECK_LT(0, size);
DCHECK_EQ(0, size % 4);
RecordUsedSpillOffset(start + size);

int max_stp_offset = -start - size;
// We check IsImmLSUnscaled(-start-12) because str only allows for unscaled
// 9-bit immediate offset [-256,256]. If start is large enough, which can
// happen when a function has many params (>=32 i64), str cannot be encoded
// properly. We can use Str, which will generate more instructions, so
// fallback to the general case below.
if (size <= 12 * kStackSlotSize &&
IsImmLSPair(max_stp_offset, kXRegSizeLog2)) {
IsImmLSPair(max_stp_offset, kXRegSizeLog2) &&
IsImmLSUnscaled(-start - 12)) {
// Special straight-line code for up to 12 slots. Generates one
// instruction per two slots (<= 7 instructions total).
STATIC_ASSERT(kStackSlotSize == kSystemPointerSize);
Expand Down
35 changes: 35 additions & 0 deletions test/cctest/wasm/test-run-wasm-simd-liftoff.cc
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,41 @@ WASM_SIMD_LIFTOFF_TEST(S8x16Shuffle_SingleOperand) {
}
}

// Exercise Liftoff's logic for zero-initializing stack slots. We were using an
// incorrect instruction for storing zeroes into the slot when the slot offset
// was too large to fit in the instruction as an immediate.
WASM_SIMD_LIFTOFF_TEST(FillStackSlotsWithZero_CheckStartOffset) {
WasmRunner<int64_t> r(ExecutionTier::kLiftoff, kNoLowerSimd);
// Function that takes in 32 i64 arguments, returns i64. This gets us a large
// enough starting offset from which we spill locals.
// start = 32 * 8 + 16 (instance) = 272 (cannot fit in signed int9).
FunctionSig* sig =
r.CreateSig<int64_t, int64_t, int64_t, int64_t, int64_t, int64_t, int64_t,
int64_t, int64_t, int64_t, int64_t, int64_t, int64_t, int64_t,
int64_t, int64_t, int64_t, int64_t, int64_t, int64_t, int64_t,
int64_t, int64_t, int64_t, int64_t, int64_t, int64_t, int64_t,
int64_t, int64_t, int64_t, int64_t, int64_t>();
WasmFunctionCompiler& simd_func = r.NewFunction(sig);

// We zero 16 bytes at a time using stp, so allocate locals such that we get a
// remainder, 8 in this case, so we hit the case where we use str.
simd_func.AllocateLocal(kWasmS128);
simd_func.AllocateLocal(kWasmI64);
BUILD(simd_func, WASM_I64V_1(1));

BUILD(r, WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1),
WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1),
WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1),
WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1),
WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1),
WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1),
WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1),
WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1), WASM_I64V_1(1),
WASM_CALL_FUNCTION0(simd_func.function_index()));

CHECK_EQ(1, r.Call());
}

#undef WASM_SIMD_LIFTOFF_TEST

} // namespace test_run_wasm_simd_liftoff
Expand Down

0 comments on commit c92e74f

Please sign in to comment.