Bug 114811

Summary: LLInt ARM backend should not use the d8 register as scratch register
Product: WebKit Reporter: Gabor Rapcsanyi <rgabor>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, commit-queue, fpizlo, oliver, ossy, sg5.lee, 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-04-18 06:40:12 PDT
LLInt ARM backend is using d8 register as scratch register but the ARM ABI says that d8-d15 registers must be preserved across subroutine calls.
Comment 1 Gabor Rapcsanyi 2013-04-18 06:42:41 PDT
Created attachment 198731 [details]
proposed fix
Comment 2 Benjamin Poulain 2013-04-18 13:31:46 PDT
I don't know much about LLInt, but why does the ABI matters at all here?
I would think the only thing that matters is follow the JIT conventions(?).
Comment 3 Filip Pizlo 2013-04-18 14:12:32 PDT
(In reply to comment #2)
> I don't know much about LLInt, but why does the ABI matters at all here?
> I would think the only thing that matters is follow the JIT conventions(?).

I think that the LLInt uses more scratch registers, and it doesn't have to use the same scratch registers as the JIT.  So this change looks valid to me.
Comment 4 WebKit Commit Bot 2013-04-18 14:55:16 PDT
Comment on attachment 198731 [details]
proposed fix

Clearing flags on attachment: 198731

Committed r148705: <http://trac.webkit.org/changeset/148705>
Comment 5 WebKit Commit Bot 2013-04-18 14:55:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 SangGyu Lee 2013-05-12 16:36:24 PDT
Is there any reason to duplicate my preceding bug report and fix (https://bugs.webkit.org/show_bug.cgi?id=114495) as new one?
Comment 7 Benjamin Poulain 2013-05-12 17:11:14 PDT
(In reply to comment #6)
> Is there any reason to duplicate my preceding bug report and fix (https://bugs.webkit.org/show_bug.cgi?id=114495) as new one?

In WebKit, we care about:
-bug report with the best patch.
-bug report with the best info.

The order in which bugs are reported does not matter. We dupe backward or forward to the best bug report.
Comment 8 Gabor Rapcsanyi 2013-05-13 00:51:19 PDT
(In reply to comment #6)
> Is there any reason to duplicate my preceding bug report and fix (https://bugs.webkit.org/show_bug.cgi?id=114495) as new one?

Sorry, I forgot to check before I reported and fixed it. I found this bug after debugging a bug for a week.
Comment 9 SangGyu Lee 2013-05-13 02:25:38 PDT
Hello, Benjamin Poulain 

> In WebKit, we care about:
> -bug report with the best patch.
> -bug report with the best info.

> The order in which bugs are reported does not matter. We dupe backward or 
> forward to the best bug report.

What do you mean by "best patch" or "best info"?

Do you mean followings?

1) I didn't make my suggested patch as attached patch.
2) I didn't request to reviewer more aggressively (by using IRC or by sending mail again and again)

I cannot find the difference between mine and this one:

title(114495-mine): LLInt should not use d8 register as scratch register
title(114811-this): LLInt ARM backend should not use the d8 register as scratch register 

and here is my bug report:

Currently, LLInt uses d8 register as scratch register as followings:

ARM_SCRATCH_FPR = SpecialRegister.new("d8")    in arm.rb
C_LOOP_SCRATCH_FPR = SpecialRegister.new("d8") in cloop.rb

However, AAPCS §5.1.2.1 says

Registers s16-s31 (d8-d15, q4-q7) must be preserved across subroutine calls; registers s0-s15 (d0-d7, q0-q3) do not need to be preserved (and can be used for passing arguments or returning results in standard procedure-call variants). Registers d16-d31 (q8-q15), if present, do not need to be preserved.

Therefore it should not use d8 for that purpose.

I think it would be safe to use d6.
(Register d7 is already in use for other purpose. )

ARM_SCRATCH_FPR = SpecialRegister.new("d6")    in arm.rb
C_LOOP_SCRATCH_FPR = SpecialRegister.new("d6") in cloop.rb
Comment 10 Csaba Osztrogonác 2013-05-13 03:20:16 PDT
Gábor ran into the same bug and then reported and fixed it quickly.
And then apologized to you because he didn't check/find if there was 
already a bug report about this bug. Additionally it isn't so easy to 
find it, because there are 15847 unconfirmed/new bugs now. If you file
a new bug report without cc-ing/poke-ing folks maybe nobody will notice
that you filed a new bug report.
Comment 11 Mark Lam 2013-07-22 15:21:10 PDT
*** Bug 114495 has been marked as a duplicate of this bug. ***