WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
213103
JIT thunks should work on arm64_32
https://bugs.webkit.org/show_bug.cgi?id=213103
Summary
JIT thunks should work on arm64_32
Keith Miller
Reported
2020-06-11 16:36:15 PDT
JIT thunks should work on arm64_32
Attachments
Patch
(28.48 KB, patch)
2020-06-11 17:00 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(30.67 KB, patch)
2020-06-12 11:12 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(31.24 KB, patch)
2020-06-12 11:36 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(31.57 KB, patch)
2020-06-12 13:08 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch for landing
(31.68 KB, patch)
2020-06-15 11:21 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2020-06-11 17:00:17 PDT
Created
attachment 401694
[details]
Patch
Keith Miller
Comment 2
2020-06-12 11:12:11 PDT
Created
attachment 401752
[details]
Patch
Keith Miller
Comment 3
2020-06-12 11:36:06 PDT
Created
attachment 401755
[details]
Patch
Keith Miller
Comment 4
2020-06-12 13:08:36 PDT
Created
attachment 401768
[details]
Patch
Darin Adler
Comment 5
2020-06-14 13:43:19 PDT
Comment on
attachment 401768
[details]
Patch Looks just right to me, but I probably shouldn't review because I’m not familiar enough with this.
Saam Barati
Comment 6
2020-06-14 21:40:43 PDT
Comment on
attachment 401768
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=401768&action=review
r=me
> Source/JavaScriptCore/ChangeLog:16 > + 2) MacroAssembler::*Ptr functions call 32/64 bit variants based on > + Address space size rather than cpu architecture.
What about call sites where they're using Ptr as a shortcut for 64-bit? Will those be vetted and changed later on?
> Source/JavaScriptCore/ChangeLog:23 > + 5) numberOfDFGCompiles should report a big number for useBaselineJIT=0
you should say why
> Source/JavaScriptCore/assembler/MacroAssembler.h:1253 > + bool shouldBlindDouble(double value)
this indentation looks off
> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:155 > + if (b == ARM64Registers::sp) > + std::swap(a, b);
is this required by the architecture?
> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2510 > void signExtend32ToPtr(TrustedImm32 imm, RegisterID dest) > { > - move(TrustedImmPtr(reinterpret_cast<void*>(static_cast<intptr_t>(imm.m_value))), dest); > + move(TrustedImm64(imm.m_value), dest); > }
Why is this needed for address32? Shouldn't we doing a 32-bit-mov?
> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:447 > + if (!imm.m_value) > + move(src, dest); > + else > + m_assembler.ror(dest, src, imm.m_value & 0x1f);
do these have the same semantics for what happen to upper bits?
Keith Miller
Comment 7
2020-06-15 11:08:09 PDT
Comment on
attachment 401768
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=401768&action=review
>> Source/JavaScriptCore/ChangeLog:16 >> + Address space size rather than cpu architecture. > > What about call sites where they're using Ptr as a shortcut for 64-bit? Will those be vetted and changed later on?
Yeah, I'm going through those as I go. I couldn't think of an automated way of checking for this with the way our current code is laid out. Ideally, we'd have something that returned the sizeof(<member>) in all our offsetOf* functions so we can assert it's the correct size.
>> Source/JavaScriptCore/ChangeLog:23 >> + 5) numberOfDFGCompiles should report a big number for useBaselineJIT=0 > > you should say why
Sure.
>> Source/JavaScriptCore/assembler/MacroAssembler.h:1253 >> + bool shouldBlindDouble(double value) > > this indentation looks off
indeed, will fix.
>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:155 >> + std::swap(a, b); > > is this required by the architecture?
Not sure, I just copied the code for add64... I didn't vet it in detail.
>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2510 >> } > > Why is this needed for address32? Shouldn't we doing a 32-bit-mov?
You sometimes need to signExtend to the full register size as the instruction operates on the full 64-bit operand. This should likely be renamed to signExtendToRegister as that is how we use for the most part but identifying which spots are which is hard.
>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:447 >> + m_assembler.ror(dest, src, imm.m_value & 0x1f); > > do these have the same semantics for what happen to upper bits?
Huh? This is only for armv7? What upper bits are you talking about?
Keith Miller
Comment 8
2020-06-15 11:21:32 PDT
Created
attachment 401912
[details]
Patch for landing
EWS
Comment 9
2020-06-15 11:56:09 PDT
Committed
r263049
: <
https://trac.webkit.org/changeset/263049
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 401912
[details]
.
Radar WebKit Bug Importer
Comment 10
2020-06-15 11:57:18 PDT
<
rdar://problem/64373326
>
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