Bug 135526

Summary:

Native inlining in FTL should not require three compiles of the same files

Product:

WebKit

Reporter:

Matthew Mirman <mmirman>

Component:

JavaScriptCore

Assignee:

Matthew Mirman <mmirman>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, fpizlo, ggaren, juergen, mmirman, ossy

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Merges the two inlining passes	none
Merges the two inlining passes	fpizlo: review+
Merges the two inlining passes	none

Matthew Mirman

Reported 2014-08-01 15:55:04 PDT

Patch forthcoming

Attachments
Merges the two inlining passes (102.48 KB, patch) 2014-08-01 16:57 PDT, Matthew Mirman	no flags	Details Formatted Diff Diff
Merges the two inlining passes (105.10 KB, patch) 2014-08-18 13:36 PDT, Matthew Mirman	fpizlo: review+	Details Formatted Diff Diff
Merges the two inlining passes (104.88 KB, patch) 2014-08-26 12:55 PDT, Matthew Mirman	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Matthew Mirman

Comment 1 2014-08-01 16:57:59 PDT

Created attachment 235925 [details] Merges the two inlining passes Also adds the AvailableExternallyLinkage assertion

Matthew Mirman

Comment 2 2014-08-18 13:36:31 PDT

Created attachment 236781 [details] Merges the two inlining passes Now intended for trunk branch.

Oliver Hunt

Comment 3 2014-08-18 13:37:41 PDT

Comment on attachment 236781 [details] Merges the two inlining passes Im not incredibly happy about making everything non-static - why is this necessary? Can we not make clang emit ir for static methods?

WebKit Commit Bot

Comment 4 2014-08-18 13:38:54 PDT

Attachment 236781 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSDataViewPrototype.cpp:66: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/build-symbol-table-index.py:52: missing whitespace around operator [pep8/E225] [5] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 5 2014-08-18 15:27:45 PDT

Comment on attachment 236781 [details] Merges the two inlining passes r=me

Matthew Mirman

Comment 6 2014-08-19 10:48:15 PDT

(In reply to comment #3) > (From update of attachment 236781 [details]) > Im not incredibly happy about making everything non-static - why is this necessary? Can we not make clang emit ir for static methods? The problem is not that clang can't emit IR, the problem is that the other IR that clang emits makes calls to those static methods, and when we inline it, we need to call out to those methods, which must not be private since they are no longer only being called from the same file.

Matthew Mirman

Comment 7 2014-08-19 12:03:25 PDT

Landed in http://trac.webkit.org/changeset/172756

Csaba Osztrogonác

Comment 8 2014-08-20 14:52:26 PDT

(In reply to comment #7) > Landed in http://trac.webkit.org/changeset/172756 And EFL buildfix landed in https://trac.webkit.org/changeset/172793. ( Just a note: It wasn't a fairplay game to land the patch when the EFL noticed the build breakage ahead of time. :-/ )

Matthew Mirman

Comment 9 2014-08-26 12:55:33 PDT

Created attachment 237168 [details] Merges the two inlining passes

Matthew Mirman

Comment 10 2014-08-26 12:59:05 PDT

Comment on attachment 237168 [details] Merges the two inlining passes Erroneous patch.

Note You need to log in before you can comment on or make changes to this bug.