[wasm-simd] Fix scalar lowering of kParameter

ngzhian · Commit Bot · commit e8832647b615 · 2020-01-29T19:00:30.000Z
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 <zhin@chromium.org> Reviewed-by: Deepti Gandluri <gdeepti@chromium.org> Reviewed-by: Andreas Haas <ahaas@chromium.org> Cr-Commit-Position: refs/heads/master@{#66032}
diff --git a/src/compiler/simd-scalar-lowering.cc b/src/compiler/simd-scalar-lowering.cc
@@ -909,29 +909,36 @@ void SimdScalarLowering::LowerNode(Node* node) {
     }
     case IrOpcode::kParameter: {
       DCHECK_EQ(1, node->InputCount());
+      int param_count = static_cast<int>(signature()->parameter_count());
       // Only exchange the node if the parameter count actually changed. We do
-      // not even have to do the default lowering because the the start node,
+      // not even have to do the default lowering because the start node,
       // the only input of a parameter node, only changes if the parameter count
       // changes.
-      if (GetParameterCountAfterLowering() !=
-          static_cast<int>(signature()->parameter_count())) {
+      if (GetParameterCountAfterLowering() != param_count) {
         int old_index = ParameterIndexOf(node->op());
+        // Parameter index 0 is the instance parameter, we will use old_index to
+        // index into the function signature, so we need to decrease it by 1.
+        --old_index;
         int new_index =
             GetParameterIndexAfterLoweringSimd128(signature(), old_index);
-        if (old_index == new_index) {
-          NodeProperties::ChangeOp(node, common()->Parameter(new_index));
+        // Similarly, the index into function signature needs to account for the
+        // instance parameter, so increase it by 1.
+        ++new_index;
+        NodeProperties::ChangeOp(node, common()->Parameter(new_index));
 
+        if (old_index < 0) {
+          break;
+        }
+
+        DCHECK(old_index < param_count);
+
+        if (signature()->GetParam(old_index) ==
+            MachineRepresentation::kSimd128) {
           Node* new_node[kNumLanes32];
-          for (int i = 0; i < kNumLanes32; ++i) {
-            new_node[i] = nullptr;
-          }
           new_node[0] = node;
-          if (signature()->GetParam(old_index) ==
-              MachineRepresentation::kSimd128) {
-            for (int i = 1; i < kNumLanes32; ++i) {
-              new_node[i] = graph()->NewNode(common()->Parameter(new_index + i),
-                                             graph()->start());
-            }
+          for (int i = 1; i < kNumLanes32; ++i) {
+            new_node[i] = graph()->NewNode(common()->Parameter(new_index + i),
+                                           graph()->start());
           }
           ReplaceNode(node, new_node, kNumLanes32);
         }
diff --git a/src/compiler/wasm-compiler.cc b/src/compiler/wasm-compiler.cc
@@ -6932,6 +6932,11 @@ wasm::WasmCompilationResult ExecuteTurbofanWasmCompilation(
     call_descriptor = GetI32WasmCallDescriptor(&zone, call_descriptor);
   }
 
+  if (ContainsSimd(func_body.sig) &&
+      (!CpuFeatures::SupportsWasmSimd128() || env->lower_simd)) {
+    call_descriptor = GetI32WasmCallDescriptorForSimd(&zone, call_descriptor);
+  }
+
   Pipeline::GenerateCodeForWasmFunction(
       &info, wasm_engine, mcgraph, call_descriptor, source_positions,
       node_origins, func_body, env->module, func_index);
diff --git a/test/mjsunit/wasm/simd-call.js b/test/mjsunit/wasm/simd-call.js
@@ -0,0 +1,66 @@
+// 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.
+
+// Flags: --experimental-wasm-simd
+
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+// Tests function calls containing s128 parameters. It also checks that
+// lowering of simd calls are correct, so we create 2 functions with s128
+// arguments: function 2 has a single s128 parameter, function 3 has a i32 then
+// s128, to ensure that the arguments in different indices are correctly lowered.
+(function TestSimd128Params() {
+  const builder = new WasmModuleBuilder();
+  builder.addImportedMemory('m', 'imported_mem', 1, 2);
+
+  builder
+    .addFunction("main", makeSig([], []))
+    .addBodyWithEnd([
+      kExprI32Const, 0,
+      kSimdPrefix, kExprS128LoadMem, 0, 0,
+      kExprCallFunction, 0x01,
+      kExprEnd,
+    ]);
+
+  // Writes s128 argument to memory starting byte 16.
+  builder
+    .addFunction("function2", makeSig([kWasmS128], []))
+    .addBodyWithEnd([
+      kExprI32Const, 16,
+      kExprLocalGet, 0,
+      kSimdPrefix, kExprS128StoreMem, 0, 0,
+      kExprI32Const, 9, // This constant doesn't matter.
+      kExprLocalGet, 0,
+      kExprCallFunction, 0x02,
+      kExprEnd,
+    ]);
+
+  // Writes s128 argument to memory starting byte 32.
+  builder
+    .addFunction("function3", makeSig([kWasmI32, kWasmS128], []))
+    .addBodyWithEnd([
+      kExprI32Const, 32,
+      kExprLocalGet, 1,
+      kSimdPrefix, kExprS128StoreMem, 0, 0,
+      kExprEnd,
+    ]);
+
+  builder.addExport('main', 0);
+  var memory = new WebAssembly.Memory({initial:1, maximum:2});
+  const instance = builder.instantiate({m: {imported_mem: memory}});
+
+  const arr = new Uint8Array(memory.buffer);
+  // Fill the initial memory with some values, this is read by main and passed
+  // as arguments to function2, and then to function3.
+  for (let i = 0; i < 16; i++) {
+    arr[i] = i * 2;
+  }
+
+  instance.exports.main();
+
+  for (let i = 0; i < 16; i++) {
+    assertEquals(arr[i], arr[i+16]);
+    assertEquals(arr[i], arr[i+32]);
+  }
+})();
