Skip to content

V
V8
Commit 6f48a0e0 authored by Ng Zhi An's avatar Ng Zhi An Committed by Commit Bot
Browse files
Options

[wasm-simd] Fix decoding of simd opcodes with immediates 

Previously, we fixed the decoding of SIMD opcodes >= 0x80 that reads an
immediate. However, we left behind a TODO for SIMD opcodes <= 0x80. This
fixes it.

Given a byte sequence such as [0xfd, 0x80, 0x80, 0x0], it decodes to the
SIMD opcode S128LoadMem (the last 3 bytes decode to 0, it is not the
most efficient encoding, but is still valid). Then, when we are decoding
the immediate memarg that follows this, we need to skip ahead 3 bytes
(opcode_length). We were not doing that previously.

This patch changes the signature of SimdLaneImmediate and
Simd8x16ShuffleImmediate to make this requirement clearer. It takes a
new argument opcode_length, which is the number of bytes the LEB encoded
opcode takes up. The pc should then be passed in unchanged.

In function-body-decoder-impl.h, we also consistently pass down
opcode_length into the various helpers, and use that value to decode
immediates.

Changes have been made to wasm-interpreter to record the opcode_length
to be passed down to helpers.

Bug: chromium:1075719
Bug: v8:10258
Change-Id: I502c9ef47d4da2abadf14218bf0da19b291ec55c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2171460Reviewed-by: 's avatarDeepti Gandluri <gdeepti@chromium.org>
Reviewed-by: 's avatarClemens Backes <clemensb@chromium.org>
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67483}
parent 5aea9c39
Hide whitespace changes
Inline Side-by-side
Showing with 74 additions and 61 deletions
src/wasm/function-body-decoder-impl.h
View file @ 6f48a0e0
......@@ -508,8 +508,12 @@ struct SimdLaneImmediate {
uint8_t lane;
uint32_t length = 1;
inline SimdLaneImmediate(Decoder* decoder, const byte* pc) {
lane = decoder->read_u8<validate>(pc + 2, "lane");
inline SimdLaneImmediate(Decoder* decoder, const byte* pc,
uint32_t opcode_length) {
// Callers should pass in pc unchanged from where the decoding happens. 1 is
// added to account for the SIMD prefix byte, and opcode_length is the
// number of bytes the LEB encoding of the SIMD opcode takes.
lane = decoder->read_u8<validate>(pc + 1 + opcode_length, "lane");
}
};
......@@ -518,9 +522,14 @@ template <Decoder::ValidateFlag validate>
struct Simd8x16ShuffleImmediate {
uint8_t shuffle[kSimd128Size] = {0};
inline Simd8x16ShuffleImmediate(Decoder* decoder, const byte* pc) {
inline Simd8x16ShuffleImmediate(Decoder* decoder, const byte* pc,
uint32_t opcode_length) {
// Callers should pass in pc unchanged from where the decoding happens. 1 is
// added to account for the SIMD prefix byte, and opcode_length is the
// number of bytes the LEB encoding of the SIMD opcode takes.
for (uint32_t i = 0; i < kSimd128Size; ++i) {
shuffle[i] = decoder->read_u8<validate>(pc + 2 + i, "shuffle");
shuffle[i] =
decoder->read_u8<validate>(pc + 1 + opcode_length + i, "shuffle");
}
}
};
......@@ -2717,8 +2726,9 @@ class WasmFullDecoder : public WasmDecoder<validate> {
return this->ok();
}
uint32_t SimdExtractLane(WasmOpcode opcode, ValueType type) {
SimdLaneImmediate<validate> imm(this, this->pc_);
uint32_t SimdExtractLane(WasmOpcode opcode, ValueType type,
uint32_t opcode_length) {
SimdLaneImmediate<validate> imm(this, this->pc_, opcode_length);
if (this->Validate(this->pc_, opcode, imm)) {
Value inputs[] = {Pop(0, kWasmS128)};
auto* result = Push(type);
......@@ -2728,8 +2738,9 @@ class WasmFullDecoder : public WasmDecoder<validate> {
return imm.length;
}
uint32_t SimdReplaceLane(WasmOpcode opcode, ValueType type) {
SimdLaneImmediate<validate> imm(this, this->pc_);
uint32_t SimdReplaceLane(WasmOpcode opcode, ValueType type,
uint32_t opcode_length) {
SimdLaneImmediate<validate> imm(this, this->pc_, opcode_length);
if (this->Validate(this->pc_, opcode, imm)) {
Value inputs[2] = {UnreachableValue(this->pc_),
UnreachableValue(this->pc_)};
......@@ -2742,8 +2753,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
return imm.length;
}
uint32_t Simd8x16ShuffleOp() {
Simd8x16ShuffleImmediate<validate> imm(this, this->pc_);
uint32_t Simd8x16ShuffleOp(uint32_t opcode_length) {
Simd8x16ShuffleImmediate<validate> imm(this, this->pc_, opcode_length);
if (this->Validate(this->pc_, imm)) {
auto input1 = Pop(1, kWasmS128);
auto input0 = Pop(0, kWasmS128);
......@@ -2758,22 +2769,17 @@ class WasmFullDecoder : public WasmDecoder<validate> {
// opcode_length is the number of bytes that this SIMD-specific opcode takes
// up in the LEB128 encoded form.
uint32_t len = 0;
// TODO(v8:10258): Most of the decodings below (like SimdExtractLane) should
// take opcode_length as a parameter, since that will determine where the
// immediate is located. However, for most of these instructions, their
// encoded opcodes take up 2 bytes, so they will not be affected by the
// variable-length encoding, and will still work correctly.
switch (opcode) {
case kExprF64x2ExtractLane: {
len = SimdExtractLane(opcode, kWasmF64);
len = SimdExtractLane(opcode, kWasmF64, opcode_length);
break;
}
case kExprF32x4ExtractLane: {
len = SimdExtractLane(opcode, kWasmF32);
len = SimdExtractLane(opcode, kWasmF32, opcode_length);
break;
}
case kExprI64x2ExtractLane: {
len = SimdExtractLane(opcode, kWasmI64);
len = SimdExtractLane(opcode, kWasmI64, opcode_length);
break;
}
case kExprI32x4ExtractLane:
......@@ -2781,36 +2787,36 @@ class WasmFullDecoder : public WasmDecoder<validate> {
case kExprI16x8ExtractLaneU:
case kExprI8x16ExtractLaneS:
case kExprI8x16ExtractLaneU: {
len = SimdExtractLane(opcode, kWasmI32);
len = SimdExtractLane(opcode, kWasmI32, opcode_length);
break;
}
case kExprF64x2ReplaceLane: {
len = SimdReplaceLane(opcode, kWasmF64);
len = SimdReplaceLane(opcode, kWasmF64, opcode_length);
break;
}
case kExprF32x4ReplaceLane: {
len = SimdReplaceLane(opcode, kWasmF32);
len = SimdReplaceLane(opcode, kWasmF32, opcode_length);
break;
}
case kExprI64x2ReplaceLane: {
len = SimdReplaceLane(opcode, kWasmI64);
len = SimdReplaceLane(opcode, kWasmI64, opcode_length);
break;
}
case kExprI32x4ReplaceLane:
case kExprI16x8ReplaceLane:
case kExprI8x16ReplaceLane: {
len = SimdReplaceLane(opcode, kWasmI32);
len = SimdReplaceLane(opcode, kWasmI32, opcode_length);
break;
}
case kExprS8x16Shuffle: {
len = Simd8x16ShuffleOp();
len = Simd8x16ShuffleOp(opcode_length);
break;
}
case kExprS128LoadMem:
len = DecodeLoadMem(LoadType::kS128Load, 1);
len = DecodeLoadMem(LoadType::kS128Load, opcode_length);
break;
case kExprS128StoreMem:
len = DecodeStoreMem(StoreType::kS128Store, 1);
len = DecodeStoreMem(StoreType::kS128Store, opcode_length);
break;
case kExprS8x16LoadSplat:
len = DecodeLoadTransformMem(LoadType::kI32Load8S,
......
src/wasm/wasm-interpreter.cc
View file @ 6f48a0e0
......@@ -2226,7 +2226,7 @@ class ThreadImpl {
}
bool ExecuteSimdOp(WasmOpcode opcode, Decoder* decoder, InterpreterCode* code,
pc_t pc, int* const len) {
pc_t pc, int* const len, uint32_t opcode_length) {
switch (opcode) {
#define SPLAT_CASE(format, sType, valType, num) \
case kExpr##format##Splat: { \
......@@ -2244,30 +2244,32 @@ class ThreadImpl {
SPLAT_CASE(I16x8, int8, int32_t, 8)
SPLAT_CASE(I8x16, int16, int32_t, 16)
#undef SPLAT_CASE
#define EXTRACT_LANE_CASE(format, name) \
case kExpr##format##ExtractLane: { \
SimdLaneImmediate<Decoder::kNoValidate> imm(decoder, code->at(pc)); \
*len += 1; \
WasmValue val = Pop(); \
Simd128 s = val.to_s128(); \
auto ss = s.to_##name(); \
Push(WasmValue(ss.val[LANE(imm.lane, ss)])); \
return true; \
#define EXTRACT_LANE_CASE(format, name) \
case kExpr##format##ExtractLane: { \
SimdLaneImmediate<Decoder::kNoValidate> imm(decoder, code->at(pc), \
opcode_length); \
*len += 1; \
WasmValue val = Pop(); \
Simd128 s = val.to_s128(); \
auto ss = s.to_##name(); \
Push(WasmValue(ss.val[LANE(imm.lane, ss)])); \
return true; \
}
EXTRACT_LANE_CASE(F64x2, f64x2)
EXTRACT_LANE_CASE(F32x4, f32x4)
EXTRACT_LANE_CASE(I64x2, i64x2)
EXTRACT_LANE_CASE(I32x4, i32x4)
#undef EXTRACT_LANE_CASE
#define EXTRACT_LANE_EXTEND_CASE(format, name, sign, type) \
case kExpr##format##ExtractLane##sign: { \
SimdLaneImmediate<Decoder::kNoValidate> imm(decoder, code->at(pc)); \
*len += 1; \
WasmValue val = Pop(); \
Simd128 s = val.to_s128(); \
auto ss = s.to_##name(); \
Push(WasmValue(static_cast<type>(ss.val[LANE(imm.lane, ss)]))); \
return true; \
#define EXTRACT_LANE_EXTEND_CASE(format, name, sign, type) \
case kExpr##format##ExtractLane##sign: { \
SimdLaneImmediate<Decoder::kNoValidate> imm(decoder, code->at(pc), \
opcode_length); \
*len += 1; \
WasmValue val = Pop(); \
Simd128 s = val.to_s128(); \
auto ss = s.to_##name(); \
Push(WasmValue(static_cast<type>(ss.val[LANE(imm.lane, ss)]))); \
return true; \
}
EXTRACT_LANE_EXTEND_CASE(I16x8, i16x8, S, int32_t)
EXTRACT_LANE_EXTEND_CASE(I16x8, i16x8, U, uint32_t)
......@@ -2489,16 +2491,17 @@ class ThreadImpl {
CMPOP_CASE(I8x16LeU, i8x16, int16, int16, 16,
static_cast<uint8_t>(a) <= static_cast<uint8_t>(b))
#undef CMPOP_CASE
#define REPLACE_LANE_CASE(format, name, stype, ctype) \
case kExpr##format##ReplaceLane: { \
SimdLaneImmediate<Decoder::kNoValidate> imm(decoder, code->at(pc)); \
*len += 1; \
WasmValue new_val = Pop(); \
WasmValue simd_val = Pop(); \
stype s = simd_val.to_s128().to_##name(); \
s.val[LANE(imm.lane, s)] = new_val.to<ctype>(); \
Push(WasmValue(Simd128(s))); \
return true; \
#define REPLACE_LANE_CASE(format, name, stype, ctype) \
case kExpr##format##ReplaceLane: { \
SimdLaneImmediate<Decoder::kNoValidate> imm(decoder, code->at(pc), \
opcode_length); \
*len += 1; \
WasmValue new_val = Pop(); \
WasmValue simd_val = Pop(); \
stype s = simd_val.to_s128().to_##name(); \
s.val[LANE(imm.lane, s)] = new_val.to<ctype>(); \
Push(WasmValue(Simd128(s))); \
return true; \
}
REPLACE_LANE_CASE(F64x2, f64x2, float2, double)
REPLACE_LANE_CASE(F32x4, f32x4, float4, float)
......@@ -2658,8 +2661,8 @@ class ThreadImpl {
return true;
}
case kExprS8x16Shuffle: {
Simd8x16ShuffleImmediate<Decoder::kNoValidate> imm(decoder,
code->at(pc));
Simd8x16ShuffleImmediate<Decoder::kNoValidate> imm(
decoder, code->at(pc), opcode_length);
*len += 16;
int16 v2 = Pop().to_s128().to_i8x16();
int16 v1 = Pop().to_s128().to_i8x16();
......@@ -3051,13 +3054,15 @@ class ThreadImpl {
// Do first check for a breakpoint, in order to set hit_break correctly.
const char* skip = " ";
int len = 1;
// We need to store this, because SIMD opcodes are LEB encoded, and later
// on when executing, we need to know where to read immediates from.
uint32_t simd_opcode_length = 0;
byte orig = code->start[pc];
WasmOpcode opcode = static_cast<WasmOpcode>(orig);
if (WasmOpcodes::IsPrefixOpcode(opcode)) {
uint32_t length;
opcode = decoder.read_prefixed_opcode<Decoder::kNoValidate>(
&code->start[pc], &length);
len += length;
&code->start[pc], &simd_opcode_length);
len += simd_opcode_length;
}
if (V8_UNLIKELY(orig == kInternalBreakpoint)) {
orig = code->orig_start[pc];
......@@ -3678,7 +3683,9 @@ class ThreadImpl {
break;
}
case kSimdPrefix: {
if (!ExecuteSimdOp(opcode, &decoder, code, pc, &len)) return;
if (!ExecuteSimdOp(opcode, &decoder, code, pc, &len,
simd_opcode_length))
return;
break;
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment