Bug 143998 – [CMake] Clean up JSC JIT options

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Michael Catanzaro]

* ENABLE_LLINT_C_LOOP is the opposite of ENABLE_JIT. It would be nonsense to have both or neither enabled, so there's no need for these to be separate options.
* Add ENABLE_DFG_JIT and have ENABLE_FTL_JIT depend on it.
* Add ENABLE_FTL_NATIVE_CALL_INLINING option, dependent on ENABLE_FTL_JIT.

If you currently build with -DENABLE_LLINT_C_LOOP=ON, you will instead want to build with -DENABLE_JIT=OFF. If you currently build with -DENABLE_JIT=ON on a platform that supports baseline JIT but not DFG JIT, you'll need to add -DENABLE_DFG_JIT=OFF.

[Filip Pizlo]

This makes a lot of sense!

[Michael Catanzaro]

Note, this patch in its current form is part of a long chain of patches (hence no r? yet), and needs to be committed alongside the patch in bug #143997 (or changed to not depend on that). Also note I don't touch the Xcode files here in an attempt to not break anything on Macs, only the CMake files, since they did not already have an ENABLE_JIT option. That would be a good follow-up.

I did remove the option from FeatureList.pm, since I think that won't break anything.

Alternative: remove the ENABLE_JIT option from the CMake port, and keep just ENABLE_LLINT_C_LOOP. I thought ENABLE_JIT was a nicer option to present to users, but the Xcode files and FeatureList.pm have ENABLE_LLINT_C_LOOP but not ENABLE_JIT, so this would make things consistent between both ports without requiring changes anywhere but the CMake files.

[Michael Catanzaro]

Created attachment 251242 [details]
Patch

[Michael Catanzaro]

I took the time to do this properly, so it no longer depends on bug #143997.

[Michael Catanzaro]

Created attachment 251248 [details]
Patch

[Michael Catanzaro]

Comment on attachment 251248 [details]
Patch

Ah but it still depends on bug #143831, whoops....

[Michael Catanzaro]

Created attachment 251359 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 251359 [details]
Patch

Clearing flags on attachment: 251359

Committed r183159: <http://trac.webkit.org/changeset/183159>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 251359 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251359&action=review

> Tools/Scripts/webkitperl/FeatureList.pm:-438
> -    { option => "cloop", desc => "Force use of the llint c loop",
> -      define => "ENABLE_LLINT_C_LOOP", default => 0, value => \$forceCLoop },

With this change we lost build-webkit's --cloop option.
Of course, --cmakeargs="-DENABLE_JIT=0" still works.

(I haven't used --cloop option of build-webkit ever and 
don't plan to do it in the future, just noticed it.)

[Michael Catanzaro]

Ah, I didn't realize that list of options was used for anything besides the default values. I think we should probably bring that back, as otherwise you won't be able to use build-webkit on various architectures.

Options: (a) convert the XCode build to use ENABLE_JIT so that we can restore the option to FeatureList.pm, or (b) revert this patch and instead remove the ENABLE_JIT option from the CMake build.

Maybe (b) is the way to go. When I wrote this patch, I did not realize that only CMake had the ENABLE_JIT option.

[Michael Catanzaro]

Created attachment 251439 [details]
[CMake] Should be possible for an option to depend on multiple options

[Michael Catanzaro]

(Ignore that bogus attachment.)

(In reply to comment #11)
> Options: (a) convert the XCode build to use ENABLE_JIT so that we can
> restore the option to FeatureList.pm, or (b) revert this patch and instead
> remove the ENABLE_JIT option from the CMake build.
> 
> Maybe (b) is the way to go. When I wrote this patch, I did not realize that
> only CMake had the ENABLE_JIT option.

Filip, Ossy, what do you prefer?

[Csaba Osztrogonác]

(In reply to comment #11)
> Ah, I didn't realize that list of options was used for anything besides the
> default values. I think we should probably bring that back, as otherwise you
> won't be able to use build-webkit on various architectures.
> 
> Options: (a) convert the XCode build to use ENABLE_JIT so that we can
> restore the option to FeatureList.pm, or (b) revert this patch and instead
> remove the ENABLE_JIT option from the CMake build.
> 
> Maybe (b) is the way to go. When I wrote this patch, I did not realize that
> only CMake had the ENABLE_JIT option.

I would prefer option (a), I think ENABLE_JIT is more descriptive
name than ENABLE_LLINT_C_LOOP. Additionally they are the opposite
of each other, why should we have two different option?

So I think we should use ENABLE_JIT everywhere,
only the following files should be changed:
- Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:464
- */FeatureDefines.xcconfig
- Platform.h:762
- build-jsc

[Michael Catanzaro]

Aye-aye, see bug #144304.