WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
114811
LLInt ARM backend should not use the d8 register as scratch register
https://bugs.webkit.org/show_bug.cgi?id=114811
Summary
LLInt ARM backend should not use the d8 register as scratch register
Gabor Rapcsanyi
Reported
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.
Attachments
proposed fix
(1.32 KB, patch)
2013-04-18 06:42 PDT
,
Gabor Rapcsanyi
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Gabor Rapcsanyi
Comment 1
2013-04-18 06:42:41 PDT
Created
attachment 198731
[details]
proposed fix
Benjamin Poulain
Comment 2
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(?).
Filip Pizlo
Comment 3
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.
WebKit Commit Bot
Comment 4
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
>
WebKit Commit Bot
Comment 5
2013-04-18 14:55:18 PDT
All reviewed patches have been landed. Closing bug.
SangGyu Lee
Comment 6
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?
Benjamin Poulain
Comment 7
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.
Gabor Rapcsanyi
Comment 8
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.
SangGyu Lee
Comment 9
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
Csaba Osztrogonác
Comment 10
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.
Mark Lam
Comment 11
2013-07-22 15:21:10 PDT
***
Bug 114495
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.
Top of Page
Format For Printing
XML
Clone This Bug