116306 2013-05-17 05:02:36 -0700 MacroAssemblerARM should use xor to swap registers instead of move 2013-06-26 07:09:49 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 108645 1 rgabor webkit-unassigned benjamin commit-queue fpizlo ossy scerveau zherczeg oldest_to_newest 890294 0 rgabor 2013-05-17 05:02:36 -0700 On ARM Traditional and Thumb2 we are using a temporary register to swap two register value. ARM Traditional using the r3 register for that purpose. MacroAssemblerARM.h:540 void swap(RegisterID reg1, RegisterID reg2) { move(reg1, ARMRegisters::S0); move(reg2, reg1); move(ARMRegisters::S0, reg2); } That causing a problem because there is a case when we set up the function parameters in DFGCCallHelpers.h we put the third parameter to r3 then in setupTwoStubArgs() we swap r1 and r2 using r3 as temporary register. DFGCCallHelpers.h:460 if (arg1 != GPRInfo::argumentGPR3 && arg2 != GPRInfo::argumentGPR3) { move(arg3, GPRInfo::argumentGPR3); setupTwoStubArgs<GPRInfo::argumentGPR1, GPRInfo::argumentGPR2>(arg1, arg2); return; } This generates the following code which is not correct: => 0x40022a3c: mov r3, r4 => 0x40022a40: mov r3, r1 => 0x40022a44: mov r1, r2 => 0x40022a48: mov r2, r3 ARM Thumb2 is using the r12 register so this bug doesn't affect it. 890298 1 202065 rgabor 2013-05-17 05:15:19 -0700 Created attachment 202065 proposed fix Maybe we should consider to make this change on ARMv7 as well. 893900 2 202065 zherczeg 2013-05-27 00:05:40 -0700 Comment on attachment 202065 proposed fix r=me 893985 3 202065 commit-queue 2013-05-27 05:22:56 -0700 Comment on attachment 202065 proposed fix Clearing flags on attachment: 202065 Committed r150748: <http://trac.webkit.org/changeset/150748> 893986 4 commit-queue 2013-05-27 05:22:58 -0700 All reviewed patches have been landed. Closing bug. 894212 5 benjamin 2013-05-27 20:55:38 -0700 (In reply to comment #1) > Maybe we should consider to make this change on ARMv7 as well. I think we should not. ARMv7 has register aliasing, making "mov" essentially free in most situation. Does the bug exist on ARMv7 though? 894214 6 benjamin 2013-05-27 20:56:05 -0700 Well, let's say, some ARMv7 have register aliasing... :) 894216 7 fpizlo 2013-05-27 21:07:44 -0700 (In reply to comment #5) > (In reply to comment #1) > > Maybe we should consider to make this change on ARMv7 as well. > > I think we should not. > ARMv7 has register aliasing, making "mov" essentially free in most situation. > > Does the bug exist on ARMv7 though? Yeah this doesn't feel right for the ARMv7 assembler. There we have scratch registers for doing this. So long as there is a spare register the moves will be faster. 894282 8 ossy 2013-05-28 03:24:03 -0700 *** Bug 112697 has been marked as a duplicate of this bug. *** 894283 9 ossy 2013-05-28 03:25:15 -0700 This bug also fixed many failing tests reported in https://bugs.webkit.org/show_bug.cgi?id=105304 896878 10 rgabor 2013-06-04 07:00:16 -0700 *** Bug 113774 has been marked as a duplicate of this bug. *** 903694 11 scerveau 2013-06-26 07:09:49 -0700 *** Bug 117349 has been marked as a duplicate of this bug. *** 202065 2013-05-17 05:15:19 -0700 2013-05-27 05:22:56 -0700 proposed fix swap.patch text/plain 1346 rgabor ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDRlOWFhN2IuLjE2MDg4MjYgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEzLTA1LTE3ICBHYWJvciBSYXBjc2Fu eWkgIDxyZ2Fib3JAd2Via2l0Lm9yZz4KKworICAgICAgICBNYWNyb0Fzc2VtYmxlckFSTSBzaG91 bGQgdXNlIHhvciB0byBzd2FwIHJlZ2lzdGVycyBpbnN0ZWFkIG9mIG1vdmUKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExNjMwNgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIENoYW5nZSByZWdpc3RlciBzd2Fw cGluZyB0byB4b3IgZnJvbSBtb3ZlIGFuZCB0aGlzIHdheSB3ZSBkb24ndCBuZWVkCisgICAgICAg IHRlbXBvcmFyeSByZWdpc3RlciBhbnltb3JlLgorCisgICAgICAgICogYXNzZW1ibGVyL01hY3Jv QXNzZW1ibGVyQVJNLmg6CisgICAgICAgIChKU0M6Ok1hY3JvQXNzZW1ibGVyQVJNOjpzd2FwKToK KwogMjAxMy0wNS0xMyAgWmFsYW4gQnVqdGFzICA8emFsYW5AYXBwbGUuY29tPgogCiAgICAgICAg IFdlYlByb2Nlc3MgY29uc3VtaW5nIHZlcnkgaGlnaCBDUFUgb24gbGlua2VkaW4uY29tCmRpZmYg LS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJN LmggYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNLmgK aW5kZXggNDQ1OWQyYS4uMTA0YmMzMCAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3Jl L2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFSTS5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk0uaApAQCAtNTM5LDkgKzUzOSw5IEBAIHB1Ymxp YzoKIAogICAgIHZvaWQgc3dhcChSZWdpc3RlcklEIHJlZzEsIFJlZ2lzdGVySUQgcmVnMikKICAg ICB7Ci0gICAgICAgIG1vdmUocmVnMSwgQVJNUmVnaXN0ZXJzOjpTMCk7Ci0gICAgICAgIG1vdmUo cmVnMiwgcmVnMSk7Ci0gICAgICAgIG1vdmUoQVJNUmVnaXN0ZXJzOjpTMCwgcmVnMik7CisgICAg ICAgIHhvcjMyKHJlZzEsIHJlZzIpOworICAgICAgICB4b3IzMihyZWcyLCByZWcxKTsKKyAgICAg ICAgeG9yMzIocmVnMSwgcmVnMik7CiAgICAgfQogCiAgICAgdm9pZCBzaWduRXh0ZW5kMzJUb1B0 cihSZWdpc3RlcklEIHNyYywgUmVnaXN0ZXJJRCBkZXN0KQo=