Bug 151297 – 64-bit in the DFG: non tail Calls unnecessarily store the argument count twice on the callee frame and tails calls unnecessarily store it once

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

We end up producing code like this:

      45:<!1:loc6>	Call(@42, @36, JS|MustGen|VarArgs|PureInt, Other, R:World, W:Heap, ExitsForExceptions, ClobbersExit, bc#33)  predicting Other
          0x24ab9c3ffd2c: mov $0x1, 0x10(%rsp)
          0x24ab9c3ffd34: mov $0x1, 0x10(%rsp)

Which is stupid.

[Saam Barati]

Created attachment 265560 [details]
patch

[Geoffrey Garen]

Comment on attachment 265560 [details]
patch

r=me

[Michael Saboff]

This patch fixed the issue of setting the argument count twice.

We also needs to set the argument count when making a tail call.  If we aren't, then file a bug.

[Saam Barati]

(In reply to comment #3)
> This patch fixed the issue of setting the argument count twice.
> 
> We also needs to set the argument count when making a tail call.  If we
> aren't, then file a bug.

We should be setting it on the caller's call frame, right?

[Saam Barati]

It looks like it's being set:
```      49:<!0:->	TailCall(@28, @23, @31, @34, @37, @40, @43, MustGen|VarArgs, R:World, W:SideState, Exits, bc#36)
          0x2268c3e00287: mov $0x10d4685b0, %rcx
          0x2268c3e00291: mov $0x0, 0x24(%rbp)
          0x2268c3e00298: mov $0x0, %r11
          0x2268c3e002a2: cmp %r11, %rcx
          0x2268c3e002a5: jnz 0x2268c3e00362
          0x2268c3e002ab: mov 0x20(%rbp), %edi
          0x2268c3e002ae: cmp $0x4, %edi
          0x2268c3e002b1: jbe 0x2268c3e002c8
          0x2268c3e002b7: add $0x6, %edi
          0x2268c3e002ba: and $0xfffffffe, %edi
          0x2268c3e002bd: imul $0x8, %edi, %edi
          0x2268c3e002c3: jmp 0x2268c3e002cd
          0x2268c3e002c8: mov $0x50, %edi
          0x2268c3e002cd: add %rbp, %rdi
          0x2268c3e002d0: sub $0x58, %rdi
          0x2268c3e002d4: mov 0x0(%rbp), %rbp
          0x2268c3e002d8: mov $0x10d43f700, %r11
          0x2268c3e002e2: mov %r11, 0x20(%rdi)
          0x2268c3e002e6: mov %rax, 0x28(%rdi)
          0x2268c3e002ea: mov %rsi, 0x30(%rdi)
          0x2268c3e002ee: mov %rdx, 0x38(%rdi)
          0x2268c3e002f2: mov $0xffff00000000001e, %r11
          0x2268c3e002fc: mov %r11, 0x40(%rdi)
          0x2268c3e00300: mov $0xffff000000000028, %r11
          0x2268c3e0030a: mov %r11, 0x48(%rdi)
          0x2268c3e0030e: movsd 0xb8(%rsp), %xmm0
          0x2268c3e00317: mov %rcx, 0x10(%rdi)
          0x2268c3e0031b: mov 0xa8(%rsp), %r15
          0x2268c3e00323: movsd %xmm0, (%rdi)
          0x2268c3e00327: mov 0x88(%rsp), %rbx
          0x2268c3e0032f: mov 0x90(%rsp), %r12
          0x2268c3e00337: mov 0x98(%rsp), %r13
          0x2268c3e0033f: mov 0xa0(%rsp), %r14
          0x2268c3e00347: mov $0x0, 0x1c(%rdi)
          0x2268c3e0034e: mov $0x6, 0x18(%rdi)
          0x2268c3e00355: mov %rdi, %rsp
          0x2268c3e00358: jmp 0x2268c3e0035d
          0x2268c3e0035d: jmp 0x2268c3e003d9
          0x2268c3e00362: mov $0x10d43f700, %r11
          0x2268c3e0036c: mov %r11, 0x18(%rsp)
          0x2268c3e00371: mov %rax, 0x20(%rsp)
          0x2268c3e00376: mov %rsi, 0x28(%rsp)
          0x2268c3e0037b: mov %rdx, 0x30(%rsp)
          0x2268c3e00380: mov $0xffff00000000001e, %r11
          0x2268c3e0038a: mov %r11, 0x38(%rsp)
          0x2268c3e0038f: mov $0xffff000000000028, %r11
          0x2268c3e00399: mov %r11, 0x40(%rsp)
          0x2268c3e0039e: mov %rcx, 0x8(%rsp)
          0x2268c3e003a3: mov -0x28(%rbp), %rbx
          0x2268c3e003a7: mov -0x20(%rbp), %r12
          0x2268c3e003ab: mov -0x18(%rbp), %r13
          0x2268c3e003af: mov -0x10(%rbp), %r14
          0x2268c3e003b3: mov -0x8(%rbp), %r15
          0x2268c3e003b7: mov $0x0, 0x14(%rsp)
          0x2268c3e003bf: mov $0x6, 0x10(%rsp)
          0x2268c3e003c7: mov %rcx, %rax
          0x2268c3e003ca: mov $0x10d7c7f80, %rdx
          0x2268c3e003d4: call 0x226883e01c60
          0x2268c3e003d9: mov $0xed, %r11d
          0x2268c3e003df: int3 
```
which is from this program:
```
"use strict";

function bar() {}
noInline(bar);

function foo(a, b, c) {
    return bar(a, b, c, 30, 40);
}
noInline(foo);
for (let i = 0; i < 1000; i++)
    foo(true, true, 10);
```

Maybe the call frame shuffler takes care of setting the argument count?

[Saam Barati]

To clarify what I mean in the bug title:
When I say tail calls don't need to set the argument count,
I mean they don't need to set it for a "callee" they're setting up,
tail calls need to set it on the frame they're reusing.

[WebKit Commit Bot]

Comment on attachment 265560 [details]
patch

Clearing flags on attachment: 265560

Committed r192465: <http://trac.webkit.org/changeset/192465>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.