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

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. ***