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
Patch (30.67 KB, patch)
2020-06-12 11:12 PDT, Keith Miller
no flags
Patch (31.24 KB, patch)
2020-06-12 11:36 PDT, Keith Miller
no flags
Patch (31.57 KB, patch)
2020-06-12 13:08 PDT, Keith Miller
no flags
Patch for landing (31.68 KB, patch)
2020-06-15 11:21 PDT, Keith Miller
no flags
Keith Miller
Comment 1 2020-06-11 17:00:17 PDT
Keith Miller
Comment 2 2020-06-12 11:12:11 PDT
Keith Miller
Comment 3 2020-06-12 11:36:06 PDT
Keith Miller
Comment 4 2020-06-12 13:08:36 PDT
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
Note You need to log in before you can comment on or make changes to this bug.