| Summary: | Disable JIT on IA-32 without SSE2 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | karogyoker2+webkit | ||||||||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Blocker | CC: | berto, bugs-noreply, commit-queue, ews-watchlist, guijemont, karogyoker2+webkit, keith_miller, mark.lam, mcatanzaro, msaboff, saam, tpopela, ysuzuki | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
karogyoker2+webkit
2018-08-10 11:50:17 PDT
Created attachment 346916 [details]
Patch
Comment on attachment 346916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346916&action=review > Source/cmake/OptionsGTK.cmake:-129 > -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ON) Hm, I would rather keep this option PUBLIC. Can you change it to reuse ENABLE_JIT_DEFAULT, like this: WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ${ENABLE_JIT_DEFAULT}) That will be fragile, since it would be broken if the variable is removed from WebKitFeatures.cmake, but I don't see a better option, so also add a warning comment in WebKitFeatures.cmake to indicate the variable is surprisingly used in OptionsGTK.cmake. Then test that it actually works, of course. > Source/cmake/WebKitFeatures.cmake:75 > - if (WTF_CPU_ARM OR WTF_CPU_ARM64 OR WTF_CPU_MIPS OR WTF_CPU_X86_64 OR WTF_CPU_X86) > + if (WTF_CPU_ARM OR WTF_CPU_ARM64 OR WTF_CPU_MIPS OR WTF_CPU_X86_64 OR WTF_CPU_X86_SSE2) This is never set in CMake, so you're disabling it for everyone. Look in the toplevel CMakeLists.txt where WTF_CPU_X86 is set. You'll need to add some test for SSE2 instruction set there to ensure it gets defined when appropriate. Created attachment 346962 [details]
Patch
Why not disabling JIT at runtime at runtime/Options.cpp? Windows already does this. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/Options.cpp#L400-L404 Created attachment 346968 [details]
Patch
(In reply to Yusuke Suzuki from comment #4) > Why not disabling JIT at runtime at runtime/Options.cpp? > Windows already does this. > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/ > Options.cpp#L400-L404 Thank you Yusuke, this is much simpler and more flexible, I tried it and it works on my Athlon XP. Comment on attachment 346968 [details]
Patch
r=me
Comment on attachment 346968 [details] Patch Clearing flags on attachment: 346968 Committed r234784: <https://trac.webkit.org/changeset/234784> All reviewed patches have been landed. Closing bug. I think we need to remove OS(WINDOWS) from here: https://github.com/WebKit/webkit/blob/89aab857234c36c97bc0696c7d5bc03b475b6c6b/Source/JavaScriptCore/runtime/Options.cpp#L52 Maybe now it works only because something else is including MacroAssemblerX86.h but normally MacroAssembler.h would include it. Maybe the headers are included because of the source files are unified. This line would need MacroAssemblerX86.h to be included: MacroAssemblerX86::supportsFloatingPoint() What do you think? Yes, please attach a follow-up patch. Reopening Created attachment 346984 [details]
Patch
Comment on attachment 346984 [details] Patch Clearing flags on attachment: 346984 Committed r234787: <https://trac.webkit.org/changeset/234787> All reviewed patches have been landed. Closing bug. |