| Summary: | [GTK] Use FTL by default when LLVM 3.7 is available | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | achristensen, akiss, dtc-llvm, fpizlo, gustavo, mcatanzaro, ossy, ysuzuki, zan | ||||||||
| Priority: | P2 | Keywords: | Gtk | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 150595 | ||||||||||
| Bug Blocks: | 143605 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Carlos Garcia Campos
2015-02-28 03:24:57 PST
Yay! :) I was thinking about similar thing for EFL few days before, but LLVM 3.6 wasn't released that time. And my idea was to make jhbuild build LLVM automatically on x86_64 and Aarch64 based on platform.machine(). (In reply to comment #1) > Yay! :) I was thinking about similar thing for EFL few > days before, but LLVM 3.6 wasn't released that time. > > And my idea was to make jhbuild build LLVM automatically > on x86_64 and Aarch64 based on platform.machine(). What's the status of AArch64 FTL on Linux? Does it pass all the tests? (In reply to comment #2) > What's the status of AArch64 FTL on Linux? Does it pass all the tests? Not all, but almost all tests. I ran tests 4-5 days ago, only 3 varargs tests failed related to FTL: stress/construct-varargs-inline.js stress/construct-varargs-no-inline.js regress/script-tests/deltablue-varargs.js I think these failures are related to bug142030, but I'm going to run a new full test tomorrow and report all failures, if we still have. note: It seems llvm-elf-add-stackmaps-arm64.patch isn't included yet in 3.6 :( (In reply to comment #4) > note: It seems llvm-elf-add-stackmaps-arm64.patch isn't included yet in 3.6 > :( really? :-( I thought all patches had been merged. (In reply to comment #5) > (In reply to comment #4) > > note: It seems llvm-elf-add-stackmaps-arm64.patch isn't included yet in 3.6 > > :( > > really? :-( I thought all patches had been merged. We could at least enable it only for x86_64, no? (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > note: It seems llvm-elf-add-stackmaps-arm64.patch isn't included yet in 3.6 > > > :( > > > > really? :-( I thought all patches had been merged. > > We could at least enable it only for x86_64, no? Sure. And we can enable it on AArch64 with updated patches. I'm going to update the patches soon and look after pushing to LLVM upstream as soon as possible. Now LLVM 3.7.0 is released. I think we can reconsider this :) Does anybody know if 3.7 includes all the patches we need? (In reply to comment #9) > Does anybody know if 3.7 includes all the patches we need? X86_64 should work fine with LLVM 3.6, but didn't check 3.7 yet. But AArch64 won't work, it seems none of the patches landed (http://reviews.llvm.org/D8257 and http://reviews.llvm.org/D8258) And there is one more problem, LLVM 3.6 stucked in an infinite loop on AArch64. I don't know if sombody fixed it, but I don't think so. ( See https://bugs.webkit.org/show_bug.cgi?id=143604#c2 and https://llvm.org/bugs/show_bug.cgi?id=23430 for details. ) Additionally JSC is completely broken now on AArch64 without FTL too - bug149061 Otherwise why do you want to enable FTL? It was a big performance regression not so long ago - bug143822. (In reply to comment #11) > Otherwise why do you want to enable FTL? It was a big > performance regression not so long ago - bug143822. This is because JSC performance is usually tuned in Apple port. So I think aligning the configuration to JSC Apple port benefits performance improvement done by Apple port. (In reply to comment #11) > Otherwise why do you want to enable FTL? It was a big > performance regression not so long ago - bug143822. And by "big performance regression" you of course mean "30% progression on two tests and 10% regression on one test". Created attachment 265471 [details]
Patch
Build failure is because it probably requires a clean build. Created attachment 265479 [details]
Updated patch
I've realized that FTL was not used when WebKitGTK+ was installed. This is because we need to install llvmForJSC too. For not installed binaries it worked because of the RPATH.
Comment on attachment 265479 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=265479&action=review Note that Apple wrote b3 to replace LLVM in the FTL, so this dependency is going to go away in the near-ish future.... > Source/JavaScriptCore/PlatformGTK.cmake:42 > + install(TARGETS llvmForJSC Hm, what is up with llvmForJSC? Why are we loading this as a shared library at runtime? I don't see why we need a shared library; surely we should be able to just add those files to the javascriptcoregtk archive? > Source/cmake/OptionsGTK.cmake:136 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${ENABLE_FTL_DEFAULT}) Move this up, since the rest are alphabetized. ;) (In reply to comment #17) > Comment on attachment 265479 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265479&action=review > > Note that Apple wrote b3 to replace LLVM in the FTL, so this dependency is > going to go away in the near-ish future.... That's still work in progress anyway. > > Source/JavaScriptCore/PlatformGTK.cmake:42 > > + install(TARGETS llvmForJSC > > Hm, what is up with llvmForJSC? Why are we loading this as a shared library > at runtime? I don't see why we need a shared library; surely we should be > able to just add those files to the javascriptcoregtk archive? https://trac.webkit.org/changeset/157260 > > Source/cmake/OptionsGTK.cmake:136 > > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${ENABLE_FTL_DEFAULT}) > > Move this up, since the rest are alphabetized. ;) Ok. Created attachment 265580 [details]
Updated patch
This should fix the EWS build. I realized that we were not using the llvm-config from the compiled llvm, but the debian one, because the compiled version installs the generic llvm-config and we were checking first llvm-config-3.5 that was found. This patch not only searches llvm-config programs but also checks the version to keep trying if we find one that is not recent enough. This keeps the FindLLVM.cmake "generic" and should work on any distro or when building from sources.
Comment on attachment 265580 [details]
Updated patch
LGTM. (But let's wait until the GTK EWS becomes green.)
(In reply to comment #20) > Comment on attachment 265580 [details] > Updated patch > > LGTM. (But let's wait until the GTK EWS becomes green.) Sure, thanks! Committed r192469: <http://trac.webkit.org/changeset/192469> I've noticed a couple of problems after landing this patch. - We now fail to find the gallium libGL.so. This is probably due to the change in the build directory. It's looking for built sources in the source directory. - JHBuild is updated all the time. This is because I changed the revision to be a tag, but we check the commit id. I'll fix both issues in follow up patches so please don't roll out yet. Fixes landed in r192471 and 192472. It also broke the GTK+ unit tests due to an api change in jhbuild, fix landed in r192474. It seems the gallium fix was not enough, but I still don't know why. If I don't manage to fix the layout tests, I'll land a patch to disable FTL instead of rolling out to make everything easier, since all other changes and follow up commits are valid anyway. Everything seems to be working again. |