Bug 116306

Summary: MacroAssemblerARM should use xor to swap registers instead of move
Product: WebKit Reporter: Gabor Rapcsanyi <rgabor>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ossy, scerveau, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645    
Attachments:
Description Flags
proposed fix none

Gabor Rapcsanyi
Reported 2013-05-17 05:02:36 PDT
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.
Attachments
proposed fix (1.31 KB, patch)
2013-05-17 05:15 PDT, Gabor Rapcsanyi 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Gabor Rapcsanyi
Comment 1 2013-05-17 05:15:19 PDT
Created attachment 202065 [details] proposed fix Maybe we should consider to make this change on ARMv7 as well.
Zoltan Herczeg
Comment 2 2013-05-27 00:05:40 PDT
Comment on attachment 202065 [details] proposed fix r=me
WebKit Commit Bot
Comment 3 2013-05-27 05:22:56 PDT
Comment on attachment 202065 [details] proposed fix Clearing flags on attachment: 202065 Committed r150748: <http://trac.webkit.org/changeset/150748>
WebKit Commit Bot
Comment 4 2013-05-27 05:22:58 PDT
All reviewed patches have been landed. Closing bug.
Benjamin Poulain
Comment 5 2013-05-27 20:55:38 PDT
(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?
Benjamin Poulain
Comment 6 2013-05-27 20:56:05 PDT
Well, let's say, some ARMv7 have register aliasing... :)
Filip Pizlo
Comment 7 2013-05-27 21:07:44 PDT
(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.
Csaba Osztrogonác
Comment 8 2013-05-28 03:24:03 PDT
*** Bug 112697 has been marked as a duplicate of this bug. ***
Csaba Osztrogonác
Comment 9 2013-05-28 03:25:15 PDT
This bug also fixed many failing tests reported in https://bugs.webkit.org/show_bug.cgi?id=105304
Gabor Rapcsanyi
Comment 10 2013-06-04 07:00:16 PDT
*** Bug 113774 has been marked as a duplicate of this bug. ***
Stephane Cerveau
Comment 11 2013-06-26 07:09:49 PDT
*** Bug 117349 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.