Summary: | Make ARM64 and X86_64 probe code a little bit more efficient. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dpino, ews-watchlist, jmason, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=229629 https://bugs.webkit.org/show_bug.cgi?id=229794 |
||||||||||
Attachments: |
|
Description
Mark Lam
2021-08-27 10:51:14 PDT
Created attachment 436642 [details]
proposed patch.
Created attachment 436643 [details]
proposed patch.
Comment on attachment 436643 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=436643&action=review > Source/JavaScriptCore/ChangeLog:18 > + For MacroAssemblerX86Common.cpp, we left the X86 and MSVC implementations unchanged. > + For X86, I don't know the stack alignment requirements (if any) plus we might want > + to delete this code eventually since we're not supporting the X86 JIT anymore. Right. > Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:-323 > -static_assert(JIT_PROBE_EXECUTOR_PTR_TAG == JITProbeExecutorPtrTag); Let's remove JITProbeExecutorPtrTag from JSCPtrTag.h since it is no longer used. (In reply to Yusuke Suzuki from comment #4) > > Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:-323 > > -static_assert(JIT_PROBE_EXECUTOR_PTR_TAG == JITProbeExecutorPtrTag); > > Let's remove JITProbeExecutorPtrTag from JSCPtrTag.h since it is no longer > used. Ah yes, I had planned to do that but forgot. Created attachment 436647 [details]
proposed patch.
Comment on attachment 436647 [details]
proposed patch.
r=me
Thanks for the review. Landed in r281718: <http://trac.webkit.org/r281718>. This patch has a bug (accidentally deleted 2 lines) that is fixed in https://bugs.webkit.org/show_bug.cgi?id=229629. This patch introduced a regression in Ubuntu 18.04. Compilation aborts on a linking error: https://build.webkit.org/#/builders/71/builds/222 FAILED: lib/libjavascriptcoregtk-4.0.so.18.19.2 : && /usr/lib/ccache/g++-8 -fPIC -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wun /usr/bin/ld.gold: error: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-cd2e8cfa-2.cpp.o: requires dynamic R_X86_64_PC32 reloc again collect2: error: ld returned 1 exit status [207/6083] Building CXX object Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/compiler/translator/tree_ops/RewriteAtomicFunctionExpressions.cpp.o ninja: build stopped: subcommand failed. program finished with exit code 254 This Ubuntu 18.04 bot uses GCC-8 (v8.4.0) and links with GNU Gold (GNU Binutils for Ubuntu 2.30 - 1.15). On the Debian bot this regression didn't happen. It uses a similar compiler and linker. Compiler: v8.3.0. Linker: GNU Binutils for Debian 2.31.1 - 1.16. Any clue or hint on what might have gone wrong? There is a Debian bot with (In reply to Diego Pino from comment #10) > This patch introduced a regression in Ubuntu 18.04. Compilation aborts on a > linking error: > ... > /usr/bin/ld.gold: error: > Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/ > DerivedSources/unified-sources/UnifiedSource-cd2e8cfa-2.cpp.o: requires > dynamic R_X86_64_PC32 reloc again > collect2: error: ld returned 1 exit status ... > Any clue or hint on what might have gone wrong? > There is a Debian bot with Looks like your linker doesn't support linking the direct branch I used in the inline asm statement in ctiMasmProbeTrampoline. I can try re-instating the old way of working for non-darwin ports since the diff isn't big in for X86_64. (In reply to Mark Lam from comment #11) > Looks like your linker doesn't support linking the direct branch I used in > the inline asm statement in ctiMasmProbeTrampoline. I can try re-instating > the old way of working for non-darwin ports since the diff isn't big in for > X86_64. See https://bugs.webkit.org/show_bug.cgi?id=229794. FYI, also getting an ld fatal error with the Sun linker. It bails on the new X86_64 inline assembly instruction `call executeJSCJITProbe` (text relocation remains against symbol executeJSCJITProbe.) If I change to `call executeJSCJITProbe@PLT`, it links and runs successfully. I am patching it locally like this: --- a/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp 2021-09-01 18:03:12.777668266 +0000 +++ b/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp 2021-09-01 19:00:37.913407269 +0000 @@ -596,7 +596,7 @@ "movq %xmm15, " STRINGIZE_VALUE_OF(PROBE_CPU_XMM15_OFFSET) "(%rsp)" "\n" "movq %rsp, %rdi" "\n" // the Probe::State* arg. - "call " SYMBOL_STRING(executeJSCJITProbe) "\n" + "call " GLOBAL_REFERENCE(executeJSCJITProbe) "\n" // Make sure the Probe::State is entirely below the result stack pointer so // that register values are still preserved when we call the initializeStack where the GLOBAL_REFERENCE macro is appending @PLT. I may be the same linker issue Diego is reporting in Ubuntu. I applied Yusuke's speculative patch (r281911) instead of my own, and can confirm it clears the issue with the Sun linker. |