151297 2015-11-15 12:05:17 -0800 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 2015-11-15 23:42:44 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 saam saam benjamin commit-queue fpizlo ggaren gskachkov keith_miller mark.lam msaboff oliver sukolsak ysuzuki oldest_to_newest 1142127 0 saam 2015-11-15 12:05:17 -0800 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. 1142128 1 265560 saam 2015-11-15 12:36:35 -0800 Created attachment 265560 patch 1142140 2 265560 ggaren 2015-11-15 15:31:47 -0800 Comment on attachment 265560 patch r=me 1142142 3 msaboff 2015-11-15 16:20:34 -0800 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. 1142144 4 saam 2015-11-15 17:05:19 -0800 (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? 1142145 5 saam 2015-11-15 17:15:36 -0800 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? 1142146 6 saam 2015-11-15 17:16:40 -0800 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. 1142170 7 265560 commit-queue 2015-11-15 23:42:39 -0800 Comment on attachment 265560 patch Clearing flags on attachment: 265560 Committed r192465: <http://trac.webkit.org/changeset/192465> 1142171 8 commit-queue 2015-11-15 23:42:44 -0800 All reviewed patches have been landed. Closing bug. 265560 2015-11-15 12:36:35 -0800 2015-11-15 23:42:39 -0800 patch a-backup.diff text/plain 1683 saam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTkyNDYzKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE1LTExLTE1ICBTYWFtIGJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IDY0LWJpdCBpbiB0aGUgREZHOiBub24gdGFpbCBDYWxscyB1bm5lY2Vzc2FyaWx5IHN0b3JlIHRo ZSBhcmd1bWVudCBjb3VudCB0d2ljZSBvbiB0aGUgY2FsbGVlIGZyYW1lIGFuZCB0YWlscyBjYWxs cyB1bm5lY2Vzc2FyaWx5IHN0b3JlIGl0IG9uY2UKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTE1MTI5NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIE5vbiB0YWlsIGNhbGxzIG9ubHkgbmVlZCB0byBzdG9yZSB0 aGUgY291bnQgb25jZS4gVGFpbCBjYWxscworICAgICAgICBuZXZlciBuZWVkIHRvIHN0b3JlIHRo ZSBjb3VudCBvbiB0aGUgY2FsbGVlIGZyYW1lIGJlY2F1c2UgdGhleQorICAgICAgICB3aWxsIGJl IHJldXNpbmcgdGhlIGNhbGxlcidzIGZyYW1lLgorCisgICAgICAgICogZGZnL0RGR1NwZWN1bGF0 aXZlSklUNjQuY3BwOgorICAgICAgICAoSlNDOjpERkc6OlNwZWN1bGF0aXZlSklUOjplbWl0Q2Fs bCk6CisKIDIwMTUtMTEtMTQgIE1pY2hhZWwgU2Fib2ZmICA8bXNhYm9mZkBhcHBsZS5jb20+CiAK ICAgICAgICAgUkVHUkVTU0lPTiAocjE5MDM3MCk6IENyYXNoVHJhY2VyOiBbVVNFUl0gY29tLmFw cGxlLldlYktpdC5XZWJDb250ZW50IGF0IGNvbS5hcHBsZS5KYXZhU2NyaXB0Q29yZTogSlNDOjpK SVRDb2RlOjpleGVjdXRlICsgMTU4CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RG R1NwZWN1bGF0aXZlSklUNjQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29y ZS9kZmcvREZHU3BlY3VsYXRpdmVKSVQ2NC5jcHAJKHJldmlzaW9uIDE5MjQ2MykKKysrIFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3VsYXRpdmVKSVQ2NC5jcHAJKHdvcmtpbmcgY29w eSkKQEAgLTc3NCw4ICs3NzQsNiBAQCB2b2lkIFNwZWN1bGF0aXZlSklUOjplbWl0Q2FsbChOb2Rl KiBub2RlCiAgICAgICAgIC8vIGFyZ3VtZW50cy4KICAgICAgICAgaW50IG51bVBhc3NlZEFyZ3Mg PSBub2RlLT5udW1DaGlsZHJlbigpIC0gMTsKIAotICAgICAgICBtX2ppdC5zdG9yZTMyKE1hY3Jv QXNzZW1ibGVyOjpUcnVzdGVkSW1tMzIobnVtUGFzc2VkQXJncyksIEpJVENvbXBpbGVyOjpjYWxs ZWVGcmFtZVBheWxvYWRTbG90KEpTU3RhY2s6OkFyZ3VtZW50Q291bnQpKTsKLQogICAgICAgICBp ZiAobm9kZS0+b3AoKSA9PSBUYWlsQ2FsbCkgewogICAgICAgICAgICAgRWRnZSBjYWxsZWVFZGdl ID0gbV9qaXQuZ3JhcGgoKS5jaGlsZChub2RlLCAwKTsKICAgICAgICAgICAgIEpTVmFsdWVPcGVy YW5kIGNhbGxlZSh0aGlzLCBjYWxsZWVFZGdlKTsK