Bug 139295 - [EFL] FTL JIT not working on ARM64
Summary: [EFL] FTL JIT not working on ARM64
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 143605
  Show dependency treegraph
 
Reported: 2014-12-05 04:58 PST by Dániel Bátyai
Modified: 2015-04-10 07:16 PDT (History)
5 users (show)

See Also:


Attachments
Patch (20.46 KB, patch)
2014-12-05 05:00 PST, Dániel Bátyai
no flags Details | Formatted Diff | Diff
Patch (20.91 KB, patch)
2014-12-09 04:23 PST, Dániel Bátyai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dániel Bátyai 2014-12-05 04:58:53 PST
Currently the FTL JIT does not work on ARM64 EFL, because the ARM64 specific code part for stack unwinding is missing.
Comment 1 Dániel Bátyai 2014-12-05 05:00:51 PST
Created attachment 242627 [details]
Patch

Added the missing code for stack unwinding and some additional small fixes
Comment 2 Akos Kiss 2014-12-05 05:54:02 PST
Comment on attachment 242627 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLCompile.cpp:68
> +#elif OS(LINUX)

This is strange to me. Why do you need to care about this code in mmAllocateCodeSection? Both EFL & GTK use LLVM 3.5.0 now, so no "older" LLVM. Did you experience LLVM emitting eh_frame as a code section? If not, I would not touch this legacy code here.

However, I'd tacke the same code in mmAllocateDataSection below, since the #if/#elif/#endif guards, as they stand now, may produce invalid C++ code after the preprocessing is done. If both OS(DARWIN) and OS(LINUX) are false then we are left with a missing "if (...) {" line and a dangling closing curly brace. So, it seems to be worthwhile to add
#else
#error "Unrecognized OS"
here, so that we are on the safe side.

> Source/JavaScriptCore/ftl/FTLCompile.cpp:635
> +#else

I'd apply the "if Linux then this, if Darwin then that, error otherwise" idea here as well, instead of assuming that if we are not on Linux then it's Darwin.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:396
> +#else

Similar to the above: make sure that we are on x86-64 and bail out otherwise.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:-737
> -    // FIXME: Implement stackunwinding based on eh_frame on ARM64

In the x86-64 code path, there are some RELEASE_ASSERTs here. Have they no counterpart on ARM64?

> Tools/efl/patches/llvm-elf-add-stackmaps.patch:60
> +

I don't think that patching up an "official" patch is a good idea. That is, both llvm-elf-add-stackmaps.patch and llvm-elf-allow-fde-references-outside-the-2gb-range.patch have been accepted to and landed in LLVM trunk. They bear their commit ID, author's name, timestamp, comments, etc. Now, this would just change the "content" of the patches leaving everything else unchanged. It may become confusing later if someone checks out the changes from the LLVM repository by commit ID and finds that there are differences.

So, I'd either replace the "official" patches with new patches that don't have references to the LLVM repository, or add two new patches in Tools/efl/patches that add whatever is missing for ARM64 to work. (I'd vote for the second option.)
Comment 3 Dániel Bátyai 2014-12-05 09:30:20 PST
(In reply to comment #2)
> Comment on attachment 242627 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242627&action=review
> 
> > Source/JavaScriptCore/ftl/FTLCompile.cpp:68
> > +#elif OS(LINUX)
> 
> This is strange to me. Why do you need to care about this code in
> mmAllocateCodeSection? Both EFL & GTK use LLVM 3.5.0 now, so no "older"
> LLVM. Did you experience LLVM emitting eh_frame as a code section? If not, I
> would not touch this legacy code here.
You're probably right, let me double-check this.
> 
> However, I'd tacke the same code in mmAllocateDataSection below, since the
> #if/#elif/#endif guards, as they stand now, may produce invalid C++ code
> after the preprocessing is done. If both OS(DARWIN) and OS(LINUX) are false
> then we are left with a missing "if (...) {" line and a dangling closing
> curly brace. So, it seems to be worthwhile to add
> #else
> #error "Unrecognized OS"
> here, so that we are on the safe side.
Good point.
> 
> > Source/JavaScriptCore/ftl/FTLCompile.cpp:635
> > +#else
> 
> I'd apply the "if Linux then this, if Darwin then that, error otherwise"
> idea here as well, instead of assuming that if we are not on Linux then it's
> Darwin.
> 
> > Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:396
> > +#else
> 
> Similar to the above: make sure that we are on x86-64 and bail out otherwise.
> 
> > Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:-737
> > -    // FIXME: Implement stackunwinding based on eh_frame on ARM64
> 
> In the x86-64 code path, there are some RELEASE_ASSERTs here. Have they no
> counterpart on ARM64?
You are correct, I shouldn't have forgot about those. My bad.
> 
> > Tools/efl/patches/llvm-elf-add-stackmaps.patch:60
> > +
> 
> I don't think that patching up an "official" patch is a good idea. That is,
> both llvm-elf-add-stackmaps.patch and
> llvm-elf-allow-fde-references-outside-the-2gb-range.patch have been accepted
> to and landed in LLVM trunk. They bear their commit ID, author's name,
> timestamp, comments, etc. Now, this would just change the "content" of the
> patches leaving everything else unchanged. It may become confusing later if
> someone checks out the changes from the LLVM repository by commit ID and
> finds that there are differences.
> 
> So, I'd either replace the "official" patches with new patches that don't
> have references to the LLVM repository, or add two new patches in
> Tools/efl/patches that add whatever is missing for ARM64 to work. (I'd vote
> for the second option.)
Okay, I'll add seperate patches for the ARM64 parts.
Comment 4 Dániel Bátyai 2014-12-09 04:23:42 PST
Created attachment 242908 [details]
Patch
Comment 5 Akos Kiss 2014-12-09 05:15:57 PST
(In reply to comment #4)
> Created attachment 242908 [details]
> Patch

Looks good to me now. (But that does not mean much. Let's see what a reviewer has to say.)
Comment 6 Gyuyoung Kim 2014-12-09 06:44:21 PST
I also think JSC reviewer should review this patch. Pizlo ?
Comment 7 Michael Saboff 2014-12-15 10:16:01 PST
Comment on attachment 242908 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2014-12-15 14:57:34 PST
Comment on attachment 242908 [details]
Patch

Clearing flags on attachment: 242908

Committed r177315: <http://trac.webkit.org/changeset/177315>
Comment 9 WebKit Commit Bot 2014-12-15 14:57:38 PST
All reviewed patches have been landed.  Closing bug.