WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED DUPLICATE of
bug 170215
171563
[ARM64] Disallowed scratch register usage when allocating stack for large frame sizes in B3::Air::generate()
https://bugs.webkit.org/show_bug.cgi?id=171563
Summary
[ARM64] Disallowed scratch register usage when allocating stack for large fra...
Zan Dobersek
Reported
2017-05-02 13:11:55 PDT
[ARM64] Disallowed scratch register usage when allocating stack for large frame sizes in B3::Air::generate()
Attachments
Patch
(4.10 KB, patch)
2017-05-02 13:38 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2017-05-02 13:25:19 PDT
When generating entrypoints for specific blocks in B3::Air::generate(), the necessary stack is allocated by subtracting the frame size value from the stack pointer register.
http://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/b3/air/AirGenerate.cpp#L214
The titular problem occurs when the frame size is 4096 or larger, meaning it can't fit into a 12-bit immediate that can still be packed into a single add or sub instruction. In that case MacroAssembler64 tries to load the immediate into the data temp register, but triggers the m_allowScratchRegister assert since that's disallowed for the whole scope of B3::Air::generate().
http://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h#L236
Zan Dobersek
Comment 2
2017-05-02 13:38:49 PDT
Created
attachment 308845
[details]
Patch
Saam Barati
Comment 3
2017-05-04 01:20:08 PDT
Comment on
attachment 308845
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=308845&action=review
> Source/JavaScriptCore/b3/air/AirGenerate.cpp:221 > + AllowMacroScratchRegisterUsage allowScratch(jit);
JF is doing the same thing in:
https://bugs.webkit.org/show_bug.cgi?id=170215
I'm ok w/ having this be its own patch. A few comments: - As I suggested to JF, I think we should use AllowMacroScratchRegisterUsageIf allowScratch(isArm64()); - I think it's worth asserting that the ScratchRegister is not pinned, otherwise, this code would be wrong.
JF Bastien
Comment 4
2017-05-04 01:23:34 PDT
> - I think it's worth asserting that the ScratchRegister is not pinned, > otherwise, this code would be wrong.
My patch does pin that register.
Saam Barati
Comment 5
2017-05-04 01:44:37 PDT
(In reply to JF Bastien from
comment #4
)
> > - I think it's worth asserting that the ScratchRegister is not pinned, > > otherwise, this code would be wrong. > > My patch does pin that register.
I feel like we kind of mean something different by your use of pinning vs the Wasm use of pinning. Perhaps it's not an interesting distinction though. What confuses me about our semantics of pinning is this: - When something claims to writePinned, we must execute it. We trust that it's updating the register as it sees fit. - When something clobbers a register, that means if we care about the value inside the register, we must take action to save its value across the thing clobbering it. What are the semantics when something claims to clobber a pinned register? Somebody who understands the semantics better I'm sure can explain to me what the expectation is in this scenario.
Zan Dobersek
Comment 6
2017-05-04 03:46:39 PDT
I'm fine with this being fixed by JF in
bug #170215
. Resolving as a duplicate of that bug. *** This bug has been marked as a duplicate of
bug 170215
***
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