[arm64] Support passing more than eight arguments to C functions

[arm64] Support passing more than eight arguments to C functions

BUG=v8:6102
Review-Url: https://codereview.chromium.org/2833463002
Cr-Commit-Position: refs/heads/master@{#44765}
Committed: https://chromium.googlesource.com/v8/v8/+/0dfb0e21feda205ba426fc55a0a468a20c4e8201

[georgia.kouveli, 2017-04-19 15:42:49]

Description was changed from

==========
[arm64] Support passing more than eight arguments to C functions

BUG=
==========

to

==========
[arm64] Support passing more than eight arguments to C functions

BUG=v8:6102
==========

[georgia.kouveli, 2017-04-19 15:42:50]

georgia.kouveli@arm.com changed reviewers:
+ bmeurer@chromium.org, jgruber@chromium.org

[jgruber, 2017-04-20 07:23:02]

Awesome, thanks for doing this! lgtm but please wait for bmeurer@'s comments.

Perhaps add a test such as:

--- a/test/cctest/test-code-stub-assembler.cc
+++ b/test/cctest/test-code-stub-assembler.cc
@@ -28,6 +28,45 @@ using compiler::CodeAssemblerVariableList;
 
 namespace {
 
+int sum9(int a0, int a1, int a2, int a3, int a4, int a5, int a6, int a7,
+         int a8) {
+  return a0 + a1 + a2 + a3 + a4 + a5 + a6 + a7 + a8;
+}
+
+}  // namespace
+
+TEST(CallCFunction9) {
+  Isolate* isolate(CcTest::InitIsolateOnce());
+
+  const int kNumParams = 0;
+  CodeAssemblerTester data(isolate, kNumParams);
+  CodeStubAssembler m(data.state());
+
+  {
+    Node* const fun_constant = m.ExternalConstant(
+        ExternalReference(reinterpret_cast<Address>(sum9), isolate));
+
+    MachineType type_intptr = MachineType::IntPtr();
+
+    Node* const result = m.CallCFunction9(
+        type_intptr, type_intptr, type_intptr, type_intptr, type_intptr,
+        type_intptr, type_intptr, type_intptr, type_intptr, type_intptr,
+        fun_constant, m.IntPtrConstant(0), m.IntPtrConstant(1),
+        m.IntPtrConstant(2), m.IntPtrConstant(3), m.IntPtrConstant(4),
+        m.IntPtrConstant(5), m.IntPtrConstant(6), m.IntPtrConstant(7),
+        m.IntPtrConstant(8));
+    m.Return(m.SmiTag(result));
+  }
+
+  Handle<Code> code = data.GenerateCode();
+  FunctionTester ft(code, kNumParams);
+
+  Handle<Object> result = ft.Call().ToHandleChecked();
+  CHECK_EQ(36, Handle<Smi>::cast(result)->value());
+}
+
+namespace {
+
 void CheckToUint32Result(uint32_t expected, Handle<Object> result) {
   const int64_t result_int64 = NumberToInt64(*result);
   const uint32_t result_uint32 = NumberToUint32(*result);

[jgruber, 2017-04-20 07:30:30]

On 2017/04/20 07:23:02, jgruber wrote:
> Awesome, thanks for doing this! lgtm but please wait for bmeurer@'s comments.
> 
> Perhaps add a test such as:
> 
> --- a/test/cctest/test-code-stub-assembler.cc
> +++ b/test/cctest/test-code-stub-assembler.cc
> @@ -28,6 +28,45 @@ using compiler::CodeAssemblerVariableList;
>  
>  namespace {
>  
> +int sum9(int a0, int a1, int a2, int a3, int a4, int a5, int a6, int a7,
> +         int a8) {
> +  return a0 + a1 + a2 + a3 + a4 + a5 + a6 + a7 + a8;
> +}
> +
> +}  // namespace
> +
> +TEST(CallCFunction9) {
> +  Isolate* isolate(CcTest::InitIsolateOnce());
> +
> +  const int kNumParams = 0;
> +  CodeAssemblerTester data(isolate, kNumParams);
> +  CodeStubAssembler m(data.state());
> +
> +  {
> +    Node* const fun_constant = m.ExternalConstant(
> +        ExternalReference(reinterpret_cast<Address>(sum9), isolate));
> +
> +    MachineType type_intptr = MachineType::IntPtr();
> +
> +    Node* const result = m.CallCFunction9(
> +        type_intptr, type_intptr, type_intptr, type_intptr, type_intptr,
> +        type_intptr, type_intptr, type_intptr, type_intptr, type_intptr,
> +        fun_constant, m.IntPtrConstant(0), m.IntPtrConstant(1),
> +        m.IntPtrConstant(2), m.IntPtrConstant(3), m.IntPtrConstant(4),
> +        m.IntPtrConstant(5), m.IntPtrConstant(6), m.IntPtrConstant(7),
> +        m.IntPtrConstant(8));
> +    m.Return(m.SmiTag(result));
> +  }
> +
> +  Handle<Code> code = data.GenerateCode();
> +  FunctionTester ft(code, kNumParams);
> +
> +  Handle<Object> result = ft.Call().ToHandleChecked();
> +  CHECK_EQ(36, Handle<Smi>::cast(result)->value());
> +}
> +
> +namespace {
> +
>  void CheckToUint32Result(uint32_t expected, Handle<Object> result) {
>    const int64_t result_int64 = NumberToInt64(*result);
>    const uint32_t result_uint32 = NumberToUint32(*result);

Actually, that test fails on arm64 (a8 is 6 but should be 8).

[jgruber, 2017-04-20 08:34:34]

On 2017/04/20 07:30:30, jgruber wrote:
> Actually, that test fails on arm64 (a8 is 6 but should be 8).

Also similar failures on arm and mips. Looks like the simulator just doesn't
handle C calls with many args correctly.
See e.g. arm64 [0] (up to 8 args) and arm [1] (up to 6 args).

[0]
https://cs.chromium.org/chromium/src/v8/src/arm64/simulator-arm64.cc?q=simula...
[1]
https://cs.chromium.org/chromium/src/v8/src/arm/simulator-arm.cc?q=simulator-...

[georgia.kouveli, 2017-04-20 10:05:58]

On 2017/04/20 08:34:34, jgruber wrote:
> On 2017/04/20 07:30:30, jgruber wrote:
> > Actually, that test fails on arm64 (a8 is 6 but should be 8).
> 
> Also similar failures on arm and mips. Looks like the simulator just doesn't
> handle C calls with many args correctly.
> See e.g. arm64 [0] (up to 8 args) and arm [1] (up to 6 args).
> 
> [0]
>
https://cs.chromium.org/chromium/src/v8/src/arm64/simulator-arm64.cc?q=simula...
> [1]
>
https://cs.chromium.org/chromium/src/v8/src/arm/simulator-arm.cc?q=simulator-...


I had actually initially added a similar test in
test/cctest/compiler/test-run-machops.cc to test my patch locally, but didn't
want to overlap with yours for adding CallCFunction9. Passing the 9th argument
on the stack works correctly on an arm64 board.

The CallCFunction tests in test/cctest/compiler/test-run-machops.cc are only
built and run natively, not on the simulator. Do we really need simulator
support for more arguments when calling an x86 function from arm/arm64 etc code?
This wouldn't be useful for the regexp patch, as far as I can tell. It also
looks like we'd need to pick a somewhat arbitrary number of arguments to handle
(the number of arguments we handle currently is probably dictated by the
builtins that need to be called?)

[jgruber, 2017-04-20 11:22:55]

jgruber@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[jgruber, 2017-04-20 11:23:47]

+mstarzinger@ since bmeurer@ is on vacation.

On 2017/04/20 10:05:58, georgia.kouveli wrote:
> On 2017/04/20 08:34:34, jgruber wrote:
> > On 2017/04/20 07:30:30, jgruber wrote:
> > > Actually, that test fails on arm64 (a8 is 6 but should be 8).
> > 
> > Also similar failures on arm and mips. Looks like the simulator just doesn't
> > handle C calls with many args correctly.
> > See e.g. arm64 [0] (up to 8 args) and arm [1] (up to 6 args).
> > 
> > [0]
> >
>
https://cs.chromium.org/chromium/src/v8/src/arm64/simulator-arm64.cc?q=simula...
> > [1]
> >
>
https://cs.chromium.org/chromium/src/v8/src/arm/simulator-arm.cc?q=simulator-...
> 
> 
> I had actually initially added a similar test in
> test/cctest/compiler/test-run-machops.cc to test my patch locally, but didn't
> want to overlap with yours for adding CallCFunction9. Passing the 9th argument
> on the stack works correctly on an arm64 board.

Feel free to add CallCFunction9 and the test here, merge conflicts should be
uncomplicated.

> The CallCFunction tests in test/cctest/compiler/test-run-machops.cc are only
> built and run natively, not on the simulator. Do we really need simulator
> support for more arguments when calling an x86 function from arm/arm64 etc
code?
> This wouldn't be useful for the regexp patch, as far as I can tell.

Right, simulator redirection is not relevant in this case: 
* We grab the generated Irregexp code from the regexp object,
* which has C-linkage of the target architecture and doesn't trigger any special
handling in the simulator.

Documenting for myself and posterity: redirection only matters in when calling
C-linkage functions of the host architecture such as in my proposed CSA test
above.
When a simulator is running:
* The ExternalReference constructor wraps the function pointer in a Redirection
(e.g. simulator-arm.cc).
* At runtime, this triggers a software interrupt that grabs the arguments and
calls into
  the host-linkage C function.

> It also
> looks like we'd need to pick a somewhat arbitrary number of arguments to
handle
> (the number of arguments we handle currently is probably dictated by the
> builtins that need to be called?)

Yes, apparently.. It'd be great if we could make simulators consistently support
the same number of args across all architectures (and add some checks to ensure
this number is not exceeded). I can look into this in a (separate) CL.

[Michael Starzinger, 2017-04-20 14:34:33]

LGTM. Thanks!

[georgia.kouveli, 2017-04-20 18:31:02]

Added CallCFunction9 and a test - PTAL.

[jgruber, 2017-04-20 18:53:12]

Still lgtm.

[jgruber, 2017-04-21 07:57:26]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 07:57:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Starzinger, 2017-04-21 08:16:18]

Yup, LGTM on the RawMachineAssembler changes. And thanks for the test!

[commit-bot: I haz the power, 2017-04-21 08:47:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 08:47:43]

Dry run: This issue passed the CQ dry run.

[georgia.kouveli, 2017-04-21 09:28:49]

The CQ bit was checked by georgia.kouveli@arm.com

[commit-bot: I haz the power, 2017-04-21 09:28:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 09:30:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492766929101250,
"parent_rev": "9a511bdf4d845212cc3e12df02fb0f7109f39598", "commit_rev":
"0dfb0e21feda205ba426fc55a0a468a20c4e8201"}

[commit-bot: I haz the power, 2017-04-21 09:30:20]

Description was changed from

==========
[arm64] Support passing more than eight arguments to C functions

BUG=v8:6102
==========

to

==========
[arm64] Support passing more than eight arguments to C functions

BUG=v8:6102

Review-Url: https://codereview.chromium.org/2833463002
Cr-Commit-Position: refs/heads/master@{#44765}
Committed:
https://chromium.googlesource.com/v8/v8/+/0dfb0e21feda205ba426fc55a0a468a20c4...
==========

[commit-bot: I haz the power, 2017-04-21 09:30:21]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/0dfb0e21feda205ba426fc55a0a468a20c4...