Skip to content

Commit d9d43b6

Browse files
ngzhianCommit Bot
ngzhian
authored and
Commit Bot
committed
Reland "[wasm-simd] Fix scalar lowering of kParameter"
This relands commit e883264. The flaky test failures seems to be related to tiering, Liftoff generating different call descriptors from TurboFan when Simd128 is unsupported (since TurboFan will lower the graph, but Liftoff can continue running simd-call.js just fine). We temporarily disable tiering for this test, until we get a proper fix, like https://crrev.com/c/2029427/, but that fix requires this change since more tests will fail without the lowering fixed. Bug: v8:10169 Bug: v8:10154 Original change's description: > [wasm-simd] Fix scalar lowering of kParameter > > Lowers the call descriptor of a wasm function if it contains simd. > > Also fixes a couple of issues with the lowering of kParameter: > - the old_index == new_index check is incorrect, it would only work if > the s128 parameter is the first parameter > - the old_index was also not adjusted to account for Parameter[0] being > the wasm instance object > - new_index needs to be adjusted to account for the instance object too > > These fixes make it more similar to the lowering of kParameter in > int64-lowering.c. > > Also add a new mjsunit test to exercise this logic. > > Bug: v8:10154 > Change-Id: Ia767a464c26a6a78fd931eab9e6897890a0904e8 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2020521 > Commit-Queue: Zhi An Ng <[email protected]> > Reviewed-by: Deepti Gandluri <[email protected]> > Reviewed-by: Andreas Haas <[email protected]> > Cr-Commit-Position: refs/heads/master@{#66032} Change-Id: I1e27825025aefc5a42aeeb87d0447d6594388fa4 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2029147 Reviewed-by: Deepti Gandluri <[email protected]> Reviewed-by: Andreas Haas <[email protected]> Commit-Queue: Zhi An Ng <[email protected]> Cr-Commit-Position: refs/heads/master@{#66072}
1 parent e8ba569 commit d9d43b6

3 files changed

Lines changed: 94 additions & 14 deletions

File tree

‎src/compiler/simd-scalar-lowering.cc‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -909,29 +909,36 @@ void SimdScalarLowering::LowerNode(Node* node) {
909909
}
910910
case IrOpcode::kParameter: {
911911
DCHECK_EQ(1, node->InputCount());
912+
int param_count = static_cast<int>(signature()->parameter_count());
912913
// Only exchange the node if the parameter count actually changed. We do
913-
// not even have to do the default lowering because the the start node,
914+
// not even have to do the default lowering because the start node,
914915
// the only input of a parameter node, only changes if the parameter count
915916
// changes.
916-
if (GetParameterCountAfterLowering() !=
917-
static_cast<int>(signature()->parameter_count())) {
917+
if (GetParameterCountAfterLowering() != param_count) {
918918
int old_index = ParameterIndexOf(node->op());
919+
// Parameter index 0 is the instance parameter, we will use old_index to
920+
// index into the function signature, so we need to decrease it by 1.
921+
--old_index;
919922
int new_index =
920923
GetParameterIndexAfterLoweringSimd128(signature(), old_index);
921-
if (old_index == new_index) {
922-
NodeProperties::ChangeOp(node, common()->Parameter(new_index));
924+
// Similarly, the index into function signature needs to account for the
925+
// instance parameter, so increase it by 1.
926+
++new_index;
927+
NodeProperties::ChangeOp(node, common()->Parameter(new_index));
923928

929+
if (old_index < 0) {
930+
break;
931+
}
932+
933+
DCHECK(old_index < param_count);
934+
935+
if (signature()->GetParam(old_index) ==
936+
MachineRepresentation::kSimd128) {
924937
Node* new_node[kNumLanes32];
925-
for (int i = 0; i < kNumLanes32; ++i) {
926-
new_node[i] = nullptr;
927-
}
928938
new_node[0] = node;
929-
if (signature()->GetParam(old_index) ==
930-
MachineRepresentation::kSimd128) {
931-
for (int i = 1; i < kNumLanes32; ++i) {
932-
new_node[i] = graph()->NewNode(common()->Parameter(new_index + i),
933-
graph()->start());
934-
}
939+
for (int i = 1; i < kNumLanes32; ++i) {
940+
new_node[i] = graph()->NewNode(common()->Parameter(new_index + i),
941+
graph()->start());
935942
}
936943
ReplaceNode(node, new_node, kNumLanes32);
937944
}

‎src/compiler/wasm-compiler.cc‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6932,6 +6932,11 @@ wasm::WasmCompilationResult ExecuteTurbofanWasmCompilation(
69326932
call_descriptor = GetI32WasmCallDescriptor(&zone, call_descriptor);
69336933
}
69346934

6935+
if (ContainsSimd(func_body.sig) &&
6936+
(!CpuFeatures::SupportsWasmSimd128() || env->lower_simd)) {
6937+
call_descriptor = GetI32WasmCallDescriptorForSimd(&zone, call_descriptor);
6938+
}
6939+
69356940
Pipeline::GenerateCodeForWasmFunction(
69366941
&info, wasm_engine, mcgraph, call_descriptor, source_positions,
69376942
node_origins, func_body, env->module, func_index);

‎test/mjsunit/wasm/simd-call.js‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
// Copyright 2020 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --experimental-wasm-simd
6+
// Flags: --nowasm-tier-up
7+
// TODO(v8/10169)
8+
9+
load('test/mjsunit/wasm/wasm-module-builder.js');
10+
11+
// Tests function calls containing s128 parameters. It also checks that
12+
// lowering of simd calls are correct, so we create 2 functions with s128
13+
// arguments: function 2 has a single s128 parameter, function 3 has a i32 then
14+
// s128, to ensure that the arguments in different indices are correctly lowered.
15+
(function TestSimd128Params() {
16+
const builder = new WasmModuleBuilder();
17+
builder.addImportedMemory('m', 'imported_mem', 1, 2);
18+
19+
builder
20+
.addFunction("main", makeSig([], []))
21+
.addBodyWithEnd([
22+
kExprI32Const, 0,
23+
kSimdPrefix, kExprS128LoadMem, 0, 0,
24+
kExprCallFunction, 0x01,
25+
kExprEnd,
26+
]);
27+
28+
// Writes s128 argument to memory starting byte 16.
29+
builder
30+
.addFunction("function2", makeSig([kWasmS128], []))
31+
.addBodyWithEnd([
32+
kExprI32Const, 16,
33+
kExprLocalGet, 0,
34+
kSimdPrefix, kExprS128StoreMem, 0, 0,
35+
kExprI32Const, 9, // This constant doesn't matter.
36+
kExprLocalGet, 0,
37+
kExprCallFunction, 0x02,
38+
kExprEnd,
39+
]);
40+
41+
// Writes s128 argument to memory starting byte 32.
42+
builder
43+
.addFunction("function3", makeSig([kWasmI32, kWasmS128], []))
44+
.addBodyWithEnd([
45+
kExprI32Const, 32,
46+
kExprLocalGet, 1,
47+
kSimdPrefix, kExprS128StoreMem, 0, 0,
48+
kExprEnd,
49+
]);
50+
51+
builder.addExport('main', 0);
52+
var memory = new WebAssembly.Memory({initial:1, maximum:2});
53+
const instance = builder.instantiate({m: {imported_mem: memory}});
54+
55+
const arr = new Uint8Array(memory.buffer);
56+
// Fill the initial memory with some values, this is read by main and passed
57+
// as arguments to function2, and then to function3.
58+
for (let i = 0; i < 16; i++) {
59+
arr[i] = i * 2;
60+
}
61+
62+
instance.exports.main();
63+
64+
for (let i = 0; i < 16; i++) {
65+
assertEquals(arr[i], arr[i+16]);
66+
assertEquals(arr[i], arr[i+32]);
67+
}
68+
})();

0 commit comments

Comments
 (0)