Bug 142128 - [GTK] Use FTL by default when LLVM 3.7 is available
Summary: [GTK] Use FTL by default when LLVM 3.7 is available
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 150595
Blocks: 143605
  Show dependency treegraph
 
Reported: 2015-02-28 03:24 PST by Carlos Garcia Campos
Modified: 2015-11-17 00:07 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 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 :-)
Comment 1 Csaba Osztrogonác 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().
Comment 2 Filip Pizlo 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?
Comment 3 Csaba Osztrogonác 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.
Comment 4 Csaba Osztrogonác 2015-03-02 08:09:37 PST
note: It seems llvm-elf-add-stackmaps-arm64.patch isn't included yet in 3.6 :(
Comment 5 Carlos Garcia Campos 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.
Comment 6 Carlos Garcia Campos 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?
Comment 7 Csaba Osztrogonác 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.
Comment 8 Yusuke Suzuki 2015-10-16 04:49:22 PDT
Now LLVM 3.7.0 is released. I think we can reconsider this :)
Comment 9 Carlos Garcia Campos 2015-10-16 04:52:00 PDT
Does anybody know if 3.7 includes all the patches we need?
Comment 10 Csaba Osztrogonác 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
Comment 11 Csaba Osztrogonác 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.
Comment 12 Yusuke Suzuki 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.
Comment 13 Filip Pizlo 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".
Comment 14 Carlos Garcia Campos 2015-11-12 23:51:57 PST
Created attachment 265471 [details]
Patch
Comment 15 Carlos Garcia Campos 2015-11-13 02:01:25 PST
Build failure is because it probably requires a clean build.
Comment 16 Carlos Garcia Campos 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.
Comment 17 Michael Catanzaro 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. ;)
Comment 18 Carlos Garcia Campos 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.
Comment 19 Carlos Garcia Campos 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.
Comment 20 Csaba Osztrogonác 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.)
Comment 21 Carlos Garcia Campos 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!
Comment 22 Carlos Garcia Campos 2015-11-16 05:02:10 PST
Committed r192469: <http://trac.webkit.org/changeset/192469>
Comment 23 Carlos Garcia Campos 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.
Comment 24 Carlos Garcia Campos 2015-11-16 08:19:18 PST
Fixes landed in r192471 and 192472.
Comment 25 Carlos Garcia Campos 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.
Comment 26 Carlos Garcia Campos 2015-11-17 00:07:49 PST
Everything seems to be working again.