[wasm] Allow import function to be any kind of callables.

src/compiler/wasm-compiler.cc

 // Copyright 2015 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.
 
 #include "src/compiler/wasm-compiler.h"
 
 #include <memory>
 
 #include "src/isolate-inl.h"
 
@@ skipping 2686 matching lines @@
   }
   Node* jsval =
       ToJS(retval, context,
            sig->return_count() == 0 ? wasm::kAstStmt : sig->GetReturn());
   Node* ret =
       graph()->NewNode(jsgraph()->common()->Return(), jsval, call, start);
 
   MergeControlToEnd(jsgraph(), ret);
 }
 
-void WasmGraphBuilder::BuildWasmToJSWrapper(Handle<JSFunction> function,
+void WasmGraphBuilder::BuildWasmToJSWrapper(Handle<JSReceiver> target,
                                             wasm::FunctionSig* sig) {
-  int js_count = function->shared()->internal_formal_parameter_count();
+  DCHECK(target->IsCallable());
+
   int wasm_count = static_cast<int>(sig->parameter_count());
   int param_count;
   if (jsgraph()->machine()->Is64()) {
     param_count = wasm_count;
   } else {
     param_count = Int64Lowering::GetParameterCountAfterLowering(sig);
   }
 
   // Build the start and the parameter nodes.
   Isolate* isolate = jsgraph()->isolate();
   CallDescriptor* desc;
   Node* start = Start(param_count + 3);
   *effect_ = start;
   *control_ = start;
   // JS context is the last parameter.
-  Node* context = HeapConstant(Handle<Context>(function->context(), isolate));
+  Node* context = HeapConstant(isolate->native_context());

[Benedikt Meurer, 2016/08/03 17:14:43]

Uhm, that looks weird. Are you really sure that the current context is the
context you want here? Can we somehow guard this, i.e. add some DCHECKs that
this is what you want? Because we had tons of bugs in the past where someone was
incorrectly using the current context from the TLS instead of the correct
context.

[ahaas, 2016/08/09 14:34:54]

Done.

   Node** args = Buffer(wasm_count + 7);
 
   bool arg_count_before_args = false;
   bool add_new_target_undefined = false;
 
   int pos = 0;
-  if (js_count == wasm_count) {
-    // exact arity match, just call the function directly.
-    desc = Linkage::GetJSCallDescriptor(graph()->zone(), false, wasm_count + 1,
+
+  // Use the Call builtin.

[titzer, 2016/08/03 16:33:49]

Always using the builtin does increase the overhead of calling WASM->JS, even in
the case of an arity match. We actually started to measure that with one of the
native benchmarks ported to WASM.

Can you add a TODO to reintroduce the fast case?

[ahaas, 2016/08/09 14:34:54]

I reintroduced this optimization now for the case where target is a JSFunction.

+  Callable callable = CodeFactory::Call(isolate);
+  args[pos++] = jsgraph()->HeapConstant(callable.code());
+  desc = Linkage::GetStubCallDescriptor(isolate, graph()->zone(),
+                                        callable.descriptor(), wasm_count + 1,
                                         CallDescriptor::kNoFlags);
-    arg_count_before_args = false;
-    add_new_target_undefined = true;
-  } else {
-    // Use the Call builtin.
-    Callable callable = CodeFactory::Call(isolate);
-    args[pos++] = jsgraph()->HeapConstant(callable.code());
-    desc = Linkage::GetStubCallDescriptor(isolate, graph()->zone(),
-                                          callable.descriptor(), wasm_count + 1,
-                                          CallDescriptor::kNoFlags);
-    arg_count_before_args = true;
-  }
+  arg_count_before_args = true;
 
-  args[pos++] = jsgraph()->Constant(function);  // JS function.
+  args[pos++] = jsgraph()->Constant(target);  // JS function.

[Benedikt Meurer, 2016/08/03 17:14:43]

Nit: Update comment.

[ahaas, 2016/08/09 14:34:54]

Done.

   if (arg_count_before_args) {
     args[pos++] = jsgraph()->Int32Constant(wasm_count);  // argument count
   }
   // Create the receiver constant (either undefined or the global proxy).
   Handle<Object> receiver(isolate->heap()->undefined_value(), isolate);
-  if (is_sloppy(function->shared()->language_mode())) {
-    receiver = Handle<Object>(function->context()->global_proxy(), isolate);
-  }
   args[pos++] = jsgraph()->Constant(receiver);
-
   // Convert WASM numbers to JS values.
   int param_index = 0;
   for (int i = 0; i < wasm_count; ++i) {
     Node* param =
         graph()->NewNode(jsgraph()->common()->Parameter(param_index++), start);
     args[pos++] = ToJS(param, context, sig->GetParam(i));

[Benedikt Meurer, 2016/08/03 17:14:43]

The ToJS should be (and seems to be) completely context independent; I'd suggest
to remove the context parameter here because it's misleading and looks like a
bug on first sight.

     if (jsgraph()->machine()->Is32() && sig->GetParam(i) == wasm::kAstI64) {
       // On 32 bit platforms we have to skip the high word of int64 parameters.
       param_index++;
     }
   }
 
   if (add_new_target_undefined) {
     args[pos++] = jsgraph()->UndefinedConstant();  // new target
   }
 
@@ skipping 442 matching lines @@
 
   if (isolate->logger()->is_logging_code_events() || isolate->is_profiling()) {
     RecordFunctionCompilation(
         CodeEventListener::FUNCTION_TAG, isolate, code, "js-to-wasm", index,
         wasm::WasmName("export"),
         module->module->GetName(func->name_offset, func->name_length));
   }
   return code;
 }
 
-Handle<Code> CompileWasmToJSWrapper(Isolate* isolate,
-                                    Handle<JSFunction> function,
+Handle<Code> CompileWasmToJSWrapper(Isolate* isolate, Handle<JSReceiver> target,
                                     wasm::FunctionSig* sig, uint32_t index,
                                     Handle<String> import_module,
                                     MaybeHandle<String> import_function) {
   //----------------------------------------------------------------------------
   // Create the Graph
   //----------------------------------------------------------------------------
   Zone zone(isolate->allocator());
   Graph graph(&zone);
   CommonOperatorBuilder common(&zone);
   MachineOperatorBuilder machine(&zone);
   JSGraph jsgraph(isolate, &graph, &common, nullptr, nullptr, &machine);
 
   Node* control = nullptr;
   Node* effect = nullptr;
 
   WasmGraphBuilder builder(&zone, &jsgraph, sig);
   builder.set_control_ptr(&control);
   builder.set_effect_ptr(&effect);
-  builder.BuildWasmToJSWrapper(function, sig);
+  builder.BuildWasmToJSWrapper(target, sig);
 
   Handle<Code> code = Handle<Code>::null();
   {
     if (FLAG_trace_turbo_graph) {  // Simple textual RPO.
       OFStream os(stdout);
       os << "-- Graph after change lowering -- " << std::endl;
       os << AsRPO(graph);
     }
 
     // Schedule and compile to machine code.
@@ skipping 238 matching lines @@
                             function_->code_start_offset),
            compile_ms);
   }
 
   return code;
 }
 
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8