WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
142128
[GTK] Use FTL by default when LLVM 3.7 is available
https://bugs.webkit.org/show_bug.cgi?id=142128
Summary
[GTK] Use FTL by default when LLVM 3.7 is available
Carlos Garcia Campos
Reported
2015-02-28 03:24:57 PST
Now that LLVM 3.6 has been released and it includes all the patches we need to use FTL, I think we could bump the llvm version in our jhbuild to 3.6 and enable FTL automatically in CMake when llvm 3.6 is found. This will make all our bots (except the 32 bit one I guess) switch to FTL automatically :-)
Attachments
Patch
(30.08 KB, patch)
2015-11-12 23:51 PST
,
Carlos Garcia Campos
no flags
Details
Formatted Diff
Diff
Updated patch
(32.68 KB, patch)
2015-11-13 05:43 PST
,
Carlos Garcia Campos
no flags
Details
Formatted Diff
Diff
Updated patch
(33.30 KB, patch)
2015-11-16 03:25 PST
,
Carlos Garcia Campos
ossy
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1
2015-03-01 12:45:46 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().
Filip Pizlo
Comment 2
2015-03-01 12:48:16 PST
(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?
Csaba Osztrogonác
Comment 3
2015-03-01 13:32:26 PST
(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.
Csaba Osztrogonác
Comment 4
2015-03-02 08:09:37 PST
note: It seems llvm-elf-add-stackmaps-arm64.patch isn't included yet in 3.6 :(
Carlos Garcia Campos
Comment 5
2015-03-02 08:12:59 PST
(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.
Carlos Garcia Campos
Comment 6
2015-03-08 01:56:30 PST
(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?
Csaba Osztrogonác
Comment 7
2015-03-09 10:38:37 PDT
(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.
Yusuke Suzuki
Comment 8
2015-10-16 04:49:22 PDT
Now LLVM 3.7.0 is released. I think we can reconsider this :)
Carlos Garcia Campos
Comment 9
2015-10-16 04:52:00 PDT
Does anybody know if 3.7 includes all the patches we need?
Csaba Osztrogonác
Comment 10
2015-10-16 05:56:22 PDT
(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
Csaba Osztrogonác
Comment 11
2015-10-16 05:58:43 PDT
Otherwise why do you want to enable FTL? It was a big performance regression not so long ago -
bug143822
.
Yusuke Suzuki
Comment 12
2015-10-16 07:35:34 PDT
(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.
Filip Pizlo
Comment 13
2015-10-27 12:33:07 PDT
(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".
Carlos Garcia Campos
Comment 14
2015-11-12 23:51:57 PST
Created
attachment 265471
[details]
Patch
Carlos Garcia Campos
Comment 15
2015-11-13 02:01:25 PST
Build failure is because it probably requires a clean build.
Carlos Garcia Campos
Comment 16
2015-11-13 05:43:04 PST
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.
Michael Catanzaro
Comment 17
2015-11-13 10:24:57 PST
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. ;)
Carlos Garcia Campos
Comment 18
2015-11-15 23:31:27 PST
(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.
Carlos Garcia Campos
Comment 19
2015-11-16 03:25:35 PST
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.
Csaba Osztrogonác
Comment 20
2015-11-16 03:42:44 PST
Comment on
attachment 265580
[details]
Updated patch LGTM. (But let's wait until the GTK EWS becomes green.)
Carlos Garcia Campos
Comment 21
2015-11-16 04:43:16 PST
(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!
Carlos Garcia Campos
Comment 22
2015-11-16 05:02:10 PST
Committed
r192469
: <
http://trac.webkit.org/changeset/192469
>
Carlos Garcia Campos
Comment 23
2015-11-16 06:52:37 PST
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.
Carlos Garcia Campos
Comment 24
2015-11-16 08:19:18 PST
Fixes landed in
r192471
and 192472.
Carlos Garcia Campos
Comment 25
2015-11-16 09:33:54 PST
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.
Carlos Garcia Campos
Comment 26
2015-11-17 00:07:49 PST
Everything seems to be working again.
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