Skip to content

Commit 5c8eccb

Browse files
MikeHolmanMikeHolman
committed
[MERGE #6260 @penzn] [Wasm SIMD] Don't use view type for v128 alignment
Merge pull request #6260 from penzn:v128-align Define natural alignment separately from ArrayBufferView for 128-bit SIMD. Don't use alignment in calculating array buffer index. Tests: minor (expected vs actual) and major (remove `align=4`) tweaks. Fixes #6258 Fixes #6259
2 parents 630f662 + 24b80b9 commit 5c8eccb

File tree

10 files changed

+107
-74
lines changed

10 files changed

+107
-74
lines changed

‎lib/Backend/IRBuilderAsmJs.cpp

Lines changed: 6 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -6748,13 +6748,11 @@ IRBuilderAsmJs::BuildAsmSimdTypedArr(Js::OpCodeAsmJs newOpcode, uint32 offset, u
67486748
IRType type = TySimd128F4;
67496749
Js::RegSlot valueRegSlot = GetRegSlotFromSimd128Reg(value);
67506750

6751-
IR::RegOpnd * maskedOpnd = nullptr;
6752-
IR::Instr * maskInstr = nullptr;
6751+
IR::RegOpnd * addrOpnd = nullptr;
67536752

67546753
Js::OpCode op = GetSimdOpcode(newOpcode);
67556754
ValueType arrayType;
67566755
bool isLd = false, isConst = false;
6757-
uint32 mask = 0;
67586756

67596757
switch (newOpcode)
67606758
{
@@ -6936,7 +6934,6 @@ IRBuilderAsmJs::BuildAsmSimdTypedArr(Js::OpCodeAsmJs newOpcode, uint32 offset, u
69366934
{
69376935
#define ARRAYBUFFER_VIEW(name, align, RegType, MemType, irSuffix) \
69386936
case Js::ArrayBufferView::TYPE_##name: \
6939-
mask = ARRAYBUFFER_VIEW_MASK(align); \
69406937
arrayType = ValueType::GetObject(ObjectType::##irSuffix##Array); \
69416938
break;
69426939
#include "Language/AsmJsArrayBufferViews.h"
@@ -6949,18 +6946,7 @@ IRBuilderAsmJs::BuildAsmSimdTypedArr(Js::OpCodeAsmJs newOpcode, uint32 offset, u
69496946
{
69506947

69516948
Js::RegSlot indexRegSlot = GetRegSlotFromIntReg(slotIndex);
6952-
6953-
if (mask)
6954-
{
6955-
// AND_I4 index, mask
6956-
maskedOpnd = IR::RegOpnd::New(TyUint32, m_func);
6957-
maskInstr = IR::Instr::New(Js::OpCode::And_I4, maskedOpnd, BuildSrcOpnd(indexRegSlot, TyInt32), IR::IntConstOpnd::New(mask, TyUint32, m_func), m_func);
6958-
6959-
}
6960-
else
6961-
{
6962-
maskedOpnd = BuildSrcOpnd(indexRegSlot, TyInt32);
6963-
}
6949+
addrOpnd = BuildSrcOpnd(indexRegSlot, TyInt32);
69646950
}
69656951

69666952
IR::Instr * instr = nullptr;
@@ -6976,11 +6962,11 @@ IRBuilderAsmJs::BuildAsmSimdTypedArr(Js::OpCodeAsmJs newOpcode, uint32 offset, u
69766962
regOpnd->SetValueType(ValueType::Simd);
69776963
if (!isConst)
69786964
{
6979-
Assert(maskedOpnd);
6965+
Assert(addrOpnd);
69806966
// Js::OpCodeAsmJs::Simd128_LdArr_I4:
69816967
// Js::OpCodeAsmJs::Simd128_LdArr_F4:
69826968
// Js::OpCodeAsmJs::Simd128_LdArr_D2:
6983-
indirOpnd = IR::IndirOpnd::New(baseOpnd, maskedOpnd, type, m_func);
6969+
indirOpnd = IR::IndirOpnd::New(baseOpnd, addrOpnd, type, m_func);
69846970
}
69856971
else
69866972
{
@@ -6997,11 +6983,11 @@ IRBuilderAsmJs::BuildAsmSimdTypedArr(Js::OpCodeAsmJs newOpcode, uint32 offset, u
69976983
regOpnd->SetValueType(ValueType::Simd);
69986984
if (!isConst)
69996985
{
7000-
Assert(maskedOpnd);
6986+
Assert(addrOpnd);
70016987
// Js::OpCodeAsmJs::Simd128_StArr_I4:
70026988
// Js::OpCodeAsmJs::Simd128_StArr_F4:
70036989
// Js::OpCodeAsmJs::Simd128_StArr_D2:
7004-
indirOpnd = IR::IndirOpnd::New(baseOpnd, maskedOpnd, type, m_func);
6990+
indirOpnd = IR::IndirOpnd::New(baseOpnd, addrOpnd, type, m_func);
70056991
}
70066992
else
70076993
{
@@ -7016,10 +7002,6 @@ IRBuilderAsmJs::BuildAsmSimdTypedArr(Js::OpCodeAsmJs newOpcode, uint32 offset, u
70167002
Assert(dataWidth >= 4 && dataWidth <= 16);
70177003
instr->dataWidth = dataWidth;
70187004
indirOpnd->SetOffset(simdOffset);
7019-
if (maskInstr)
7020-
{
7021-
AddInstr(maskInstr, offset);
7022-
}
70237005
AddInstr(instr, offset);
70247006

70257007
}

‎lib/Runtime/Language/AsmJsArrayBufferViews.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#endif
1818

1919
// (Name , Align , RegType, MemType , irSuffix )
20+
// (log2)
2021
ARRAYBUFFER_VIEW_INT(INT8 , 0 , int32 , int8 , Int8 )
2122
ARRAYBUFFER_VIEW_INT(UINT8 , 0 , int32 , uint8 , Uint8 )
2223
ARRAYBUFFER_VIEW_INT(INT16 , 1 , int32 , int16 , Int16 )

‎lib/Runtime/Language/InterpreterStackFrame.cpp

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8161,7 +8161,12 @@ namespace Js
81618161
void InterpreterStackFrame::OP_SimdLdArrGeneric(const unaligned T* playout)
81628162
{
81638163
Assert(playout->ViewType < Js::ArrayBufferView::TYPE_COUNT);
8164-
const uint64 index = ((uint64)(uint32)GetRegRawInt(playout->SlotIndex) + playout->Offset /* WASM only */) & (int64)(int)ArrayBufferView::ViewMask[playout->ViewType];
8164+
8165+
if (GetRegRawInt(playout->SlotIndex) < 0) {
8166+
JavascriptError::ThrowRangeError(scriptContext, JSERR_ArgumentOutOfRange, _u("Simd typed array access"));
8167+
}
8168+
8169+
const uint64 index = (uint64)GetRegRawInt(playout->SlotIndex) + playout->Offset;
81658170

81668171
ArrayBufferBase* arr =
81678172
#ifdef ENABLE_WASM_SIMD
@@ -8209,7 +8214,12 @@ namespace Js
82098214
void InterpreterStackFrame::OP_SimdStArrGeneric(const unaligned T* playout)
82108215
{
82118216
Assert(playout->ViewType < Js::ArrayBufferView::TYPE_COUNT);
8212-
const uint64 index = ((uint64)(uint32)GetRegRawInt(playout->SlotIndex) + playout->Offset /* WASM only */) & (int64)(int)ArrayBufferView::ViewMask[playout->ViewType];
8217+
8218+
if (GetRegRawInt(playout->SlotIndex) < 0)
8219+
{
8220+
JavascriptError::ThrowRangeError(scriptContext, JSERR_ArgumentOutOfRange, _u("Simd typed array access"));
8221+
}
8222+
const uint64 index = (uint64)GetRegRawInt(playout->SlotIndex) + playout->Offset;
82138223

82148224
ArrayBufferBase* arr =
82158225
#ifdef ENABLE_WASM_SIMD

‎lib/WasmReader/WasmByteCodeGenerator.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1604,11 +1604,11 @@ EmitInfo WasmBytecodeGenerator::EmitSimdMemAccess(Js::OpCodeAsmJs op, const Wasm
16041604
WasmTypes::WasmType type = signature[0];
16051605
SetUsesMemory(0);
16061606

1607-
const uint32 mask = Js::ArrayBufferView::ViewMask[viewType];
1607+
const uint32 naturalAlignment = 16;
16081608
const uint alignment = GetReader()->m_currentNode.mem.alignment;
16091609
const uint offset = GetReader()->m_currentNode.mem.offset;
16101610

1611-
if ((mask << 1) & (1 << alignment))
1611+
if (alignment > naturalAlignment)
16121612
{
16131613
throw WasmCompilationException(_u("alignment must not be larger than natural"));
16141614
}

‎test/wasm.simd/loadTests.js

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ function testLoadOpsForType(funcname, module, laneValues, expectedResults, start
1717
const instance = new WebAssembly.Instance(module, { "dummy" : { "memory" : memObj } }).exports;
1818

1919
let intArray = new Int32Array (memObj.buffer);
20+
let int8Array = new Int8Array (memObj.buffer);
2021

2122
let forEachTestPosition = (action) => {
2223

@@ -29,7 +30,7 @@ function testLoadOpsForType(funcname, module, laneValues, expectedResults, start
2930

3031
forEachTestPosition ((pos, i) => {intArray[pos + i] = laneValues[i];});
3132
instance[funcname](0);
32-
forEachTestPosition((pos, i) => {assertEquals(intArray[pos + i], expectedResults[i]);});
33+
forEachTestPosition((pos, i) => {assertEquals(expectedResults[i], intArray[pos + i]);});
3334

3435
const MEM_SIZE_IN_BYTES = 1024 * 64;
3536

@@ -60,12 +61,19 @@ function testLoadOpsForType(funcname, module, laneValues, expectedResults, start
6061
}
6162
}
6263

63-
check(0, "instance.m128_load4", MEM_SIZE_IN_BYTES - 32);
64-
check(0, "instance.m128_load4", MEM_SIZE_IN_BYTES - 16);
65-
check("Access index is out of range", "instance.m128_load4", MEM_SIZE_IN_BYTES - 8);
66-
check("Access index is out of range", "instance.m128_load4", MEM_SIZE_IN_BYTES - 4);
67-
check("Access index is out of range", "instance.m128_load4_offset", 0xFFFFFFFC);
68-
check("Access index is out of range", "instance.m128_load4_offset", -1);
64+
check(0, "instance.v128_load4", MEM_SIZE_IN_BYTES - 32);
65+
check(0, "instance.v128_load4", MEM_SIZE_IN_BYTES - 16);
66+
check("Access index is out of range", "instance.v128_load4", MEM_SIZE_IN_BYTES - 8);
67+
check("Access index is out of range", "instance.v128_load4", MEM_SIZE_IN_BYTES - 4);
68+
check("Access index is out of range", "instance.v128_load4_offset", 0xFFFFFFFC);
69+
check("Access index is out of range", "instance.v128_load4_offset", -1);
70+
// Verify read at "odd" index
71+
int8Array[0] = 1;
72+
int8Array[1] = 0;
73+
int8Array[2] = 0;
74+
int8Array[3] = 0;
75+
int8Array[4] = 0;
76+
check(0, "instance.v128_load4", 1);
6977

7078
}
7179

@@ -76,8 +84,8 @@ const laneValues = [0xAAAAAAAA, 0xFFFFFFFF, 0X80000000, 0x90A762A6];
7684
const expectedResults = [16, 32, 1, 14]; //i32.popcnt
7785
const startPositions = [0, 5, 11, 17];
7886

79-
testLoadOpsForType("m128_load_test", module, laneValues, expectedResults,startPositions);
87+
testLoadOpsForType("v128_load_test", module, laneValues, expectedResults, startPositions);
8088

8189
if (passed) {
8290
print("Passed");
83-
}
91+
}

‎test/wasm.simd/loads.wasm

0 Bytes
Binary file not shown.

‎test/wasm.simd/loads.wast

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,49 +6,49 @@
66
(module
77
(import "dummy" "memory" (memory 1))
88

9-
(func (export "m128_load4") (param $x i32) (result i32)
10-
(i32x4.extract_lane 0 (v128.load offset=0 align=4 (get_local $x)))
9+
(func (export "v128_load4") (param $x i32) (result i32)
10+
(i32x4.extract_lane 0 (v128.load offset=0 (get_local $x)))
1111
)
1212

13-
(func (export "m128_load4_offset") (param $x i32) (result i32)
14-
(i32x4.extract_lane 0 (v128.load offset=16 align=4 (get_local $x)))
13+
(func (export "v128_load4_offset") (param $x i32) (result i32)
14+
(i32x4.extract_lane 0 (v128.load offset=16 (get_local $x)))
1515
)
1616

17-
(func (export "m128_load_test") (param $x i32) (local v128)
18-
(set_local 1 (v128.load offset=0 align=4 (get_local $x)))
17+
(func (export "v128_load_test") (param $x i32) (local v128)
18+
(set_local 1 (v128.load offset=0 (get_local $x)))
1919
(i32.store offset=0 (get_local $x) (i32.popcnt (i32x4.extract_lane 0 (get_local 1))))
20-
(set_local 1 (v128.load offset=0 align=4 (get_local $x)))
20+
(set_local 1 (v128.load offset=0 (get_local $x)))
2121
(i32.store offset=4 (get_local $x) (i32.popcnt (i32x4.extract_lane 1 (get_local 1))))
22-
(set_local 1 (v128.load offset=0 align=4 (get_local $x)))
22+
(set_local 1 (v128.load offset=0 (get_local $x)))
2323
(i32.store offset=8 (get_local $x) (i32.popcnt (i32x4.extract_lane 2 (get_local 1))))
24-
(set_local 1 (v128.load offset=0 align=4 (get_local $x)))
24+
(set_local 1 (v128.load offset=0 (get_local $x)))
2525
(i32.store offset=12 (get_local $x) (i32.popcnt (i32x4.extract_lane 3 (get_local 1))))
2626
;;
27-
(set_local 1 (v128.load offset=20 align=4 (get_local $x)))
27+
(set_local 1 (v128.load offset=20 (get_local $x)))
2828
(i32.store offset=20 (get_local $x) (i32.popcnt (i32x4.extract_lane 0 (get_local 1))))
29-
(set_local 1 (v128.load offset=20 align=4 (get_local $x)))
29+
(set_local 1 (v128.load offset=20 (get_local $x)))
3030
(i32.store offset=24 (get_local $x) (i32.popcnt (i32x4.extract_lane 1 (get_local 1))))
31-
(set_local 1 (v128.load offset=20 align=4 (get_local $x)))
31+
(set_local 1 (v128.load offset=20 (get_local $x)))
3232
(i32.store offset=28 (get_local $x) (i32.popcnt (i32x4.extract_lane 2 (get_local 1))))
33-
(set_local 1 (v128.load offset=20 align=4 (get_local $x)))
33+
(set_local 1 (v128.load offset=20 (get_local $x)))
3434
(i32.store offset=32 (get_local $x) (i32.popcnt (i32x4.extract_lane 3 (get_local 1))))
3535
;;
36-
(set_local 1 (v128.load offset=44 align=4 (get_local $x)))
36+
(set_local 1 (v128.load offset=44 (get_local $x)))
3737
(i32.store offset=44 (get_local $x) (i32.popcnt (i32x4.extract_lane 0 (get_local 1))))
38-
(set_local 1 (v128.load offset=44 align=4 (get_local $x)))
38+
(set_local 1 (v128.load offset=44 (get_local $x)))
3939
(i32.store offset=48 (get_local $x) (i32.popcnt (i32x4.extract_lane 1 (get_local 1))))
40-
(set_local 1 (v128.load offset=44 align=4 (get_local $x)))
40+
(set_local 1 (v128.load offset=44 (get_local $x)))
4141
(i32.store offset=52 (get_local $x) (i32.popcnt (i32x4.extract_lane 2 (get_local 1))))
42-
(set_local 1 (v128.load offset=44 align=4 (get_local $x)))
42+
(set_local 1 (v128.load offset=44 (get_local $x)))
4343
(i32.store offset=56 (get_local $x) (i32.popcnt (i32x4.extract_lane 3 (get_local 1))))
4444
;;
45-
(set_local 1 (v128.load offset=68 align=4 (get_local $x)))
45+
(set_local 1 (v128.load offset=68 (get_local $x)))
4646
(i32.store offset=68 (get_local $x) (i32.popcnt (i32x4.extract_lane 0 (get_local 1))))
47-
(set_local 1 (v128.load offset=68 align=4 (get_local $x)))
47+
(set_local 1 (v128.load offset=68 (get_local $x)))
4848
(i32.store offset=72 (get_local $x) (i32.popcnt (i32x4.extract_lane 1 (get_local 1))))
49-
(set_local 1 (v128.load offset=68 align=4 (get_local $x)))
49+
(set_local 1 (v128.load offset=68 (get_local $x)))
5050
(i32.store offset=76 (get_local $x) (i32.popcnt (i32x4.extract_lane 2 (get_local 1))))
51-
(set_local 1 (v128.load offset=68 align=4 (get_local $x)))
51+
(set_local 1 (v128.load offset=68 (get_local $x)))
5252
(i32.store offset=80 (get_local $x) (i32.popcnt (i32x4.extract_lane 3 (get_local 1))))
5353
)
5454
)

‎test/wasm.simd/storeTests.js

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ const module = new WebAssembly.Module(readbuffer('stores.wasm'));
3535
let memObj = new WebAssembly.Memory({initial:INITIAL_SIZE});
3636
const instance = new WebAssembly.Instance(module, { "dummy" : { "memory" : memObj } }).exports;
3737
let intArray = new Int32Array (memObj.buffer);
38+
let byteArray = new Uint8Array (memObj.buffer);
3839

3940
let testStore = function (funcname, ...expected) {
4041

@@ -51,19 +52,44 @@ let testStore = function (funcname, ...expected) {
5152
}
5253

5354

54-
testStore("m128_store4", 777, 888, 999, 1111);
55-
testStore("m128_store4", -1, 0, 0, -1);
56-
testStore("m128_store4", -1, -1, -1, -1);
55+
testStore("v128_store4", 777, 888, 999, 1111);
56+
testStore("v128_store4", -1, 0, 0, -1);
57+
testStore("v128_store4", -1, -1, -1, -1);
5758

59+
for (let i = 0; i < 18; i++) {
60+
byteArray[i] = 0;
61+
}
62+
63+
// Check that v128 store does not overrun value boundaries
64+
instance.v128_store_i32x4(1, 0x01020304);
65+
66+
assertEquals(0, byteArray[0]);
67+
assertEquals(4, byteArray[1]);
68+
assertEquals(3, byteArray[2]);
69+
assertEquals(2, byteArray[3]);
70+
assertEquals(1, byteArray[4]);
71+
assertEquals(4, byteArray[5]);
72+
assertEquals(3, byteArray[6]);
73+
assertEquals(2, byteArray[7]);
74+
assertEquals(1, byteArray[8]);
75+
assertEquals(4, byteArray[9]);
76+
assertEquals(3, byteArray[10]);
77+
assertEquals(2, byteArray[11]);
78+
assertEquals(1, byteArray[12]);
79+
assertEquals(4, byteArray[13]);
80+
assertEquals(3, byteArray[14]);
81+
assertEquals(2, byteArray[15]);
82+
assertEquals(1, byteArray[16]);
83+
assertEquals(0, byteArray[17]);
5884

5985
const MEM_SIZE_IN_BYTES = 1024 * 64;
60-
check("RangeError", "instance.m128_store4", MEM_SIZE_IN_BYTES - 12, 777, 888, 999, 1111);
61-
check("RangeError", "instance.m128_store4", MEM_SIZE_IN_BYTES - 8, 777, 888, 999, 1111);
62-
check("RangeError", "instance.m128_store4", MEM_SIZE_IN_BYTES - 4, 777, 888, 999, 1111);
63-
check("RangeError", "instance.m128_store4_offset", -1, 777, 888, 999, 1111);
64-
check("RangeError", "instance.m128_store4_offset", 0xFFFFFFFC, 777, 888, 999, 1111);
65-
check(undefined, "instance.m128_store4", MEM_SIZE_IN_BYTES - 16, 777, 888, 999, 1111);
86+
check("RangeError", "instance.v128_store4", MEM_SIZE_IN_BYTES - 12, 777, 888, 999, 1111);
87+
check("RangeError", "instance.v128_store4", MEM_SIZE_IN_BYTES - 8, 777, 888, 999, 1111);
88+
check("RangeError", "instance.v128_store4", MEM_SIZE_IN_BYTES - 4, 777, 888, 999, 1111);
89+
check("RangeError", "instance.v128_store4_offset", -1, 777, 888, 999, 1111);
90+
check("RangeError", "instance.v128_store4_offset", 0xFFFFFFFC, 777, 888, 999, 1111);
91+
check(undefined, "instance.v128_store4", MEM_SIZE_IN_BYTES - 16, 777, 888, 999, 1111);
6692

6793
if (passed) {
6894
print("Passed");
69-
}
95+
}

‎test/wasm.simd/stores.wasm

38 Bytes
Binary file not shown.

‎test/wasm.simd/stores.wast

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,19 @@
66
(module
77
(import "dummy" "memory" (memory 1))
88

9-
(func (export "m128_store4") (param i32 i32 i32 i32 i32) (local v128)
10-
(set_local 5 (v128.load offset=0 align=4 (i32.const 16)))
11-
(v128.store offset=0 align=4 (get_local 0) (get_local 5))
9+
(func (export "v128_store4") (param i32 i32 i32 i32 i32) (local v128)
10+
(set_local 5 (v128.load offset=0 (i32.const 16)))
11+
(v128.store offset=0 (get_local 0) (get_local 5))
1212
)
1313

14-
(func (export "m128_store4_offset") (param i32 i32 i32 i32 i32) (local v128)
15-
(set_local 5 (v128.load offset=0 align=4 (i32.const 16)))
16-
(v128.store offset=16 align=4 (get_local 0) (get_local 5))
14+
(func (export "v128_store4_offset") (param i32 i32 i32 i32 i32) (local v128)
15+
(set_local 5 (v128.load offset=0 (i32.const 16)))
16+
(v128.store offset=16 (get_local 0) (get_local 5))
17+
)
18+
(func
19+
(export "v128_store_i32x4")
20+
(param $index i32)
21+
(param $value i32)
22+
(v128.store align=4 (get_local $index) (i32x4.splat (get_local $value)))
1723
)
1824
)

0 commit comments

Comments
 (0)