Bug 116306 - MacroAssemblerARM should use xor to swap registers instead of move
Summary: MacroAssemblerARM should use xor to swap registers instead of move
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 112697 113774 117349 (view as bug list)
Depends on:
Blocks: 108645
  Show dependency tree / graph
 
Reported: 2013-05-17 05:02 PDT by Gabor Rapcsanyi
Modified: 2013-06-26 07:09 PDT (History)
6 users (show)

See Also:

Attachments
proposed fix (1.31 KB, patch)
2013-05-17 05:15 PDT, Gabor Rapcsanyi 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Rapcsanyi 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.
Comment 1 Gabor Rapcsanyi 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.
Comment 2 Zoltan Herczeg 2013-05-27 00:05:40 PDT 
Comment on attachment 202065 [details]
proposed fix

r=me
Comment 3 WebKit Commit Bot 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>
Comment 4 WebKit Commit Bot 2013-05-27 05:22:58 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 5 Benjamin Poulain 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?
Comment 6 Benjamin Poulain 2013-05-27 20:56:05 PDT 
Well, let's say, some ARMv7 have register aliasing... :)
Comment 7 Filip Pizlo 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.
Comment 8 Csaba Osztrogonác 2013-05-28 03:24:03 PDT 
*** Bug 112697 has been marked as a duplicate of this bug. ***
Comment 9 Csaba Osztrogonác 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
Comment 10 Gabor Rapcsanyi 2013-06-04 07:00:16 PDT 
*** Bug 113774 has been marked as a duplicate of this bug. ***
Comment 11 Stephane Cerveau 2013-06-26 07:09:49 PDT 
*** Bug 117349 has been marked as a duplicate of this bug. ***