Bug 138499 - Enable Cortex-A53-specific code paths by default if core is detected
Summary: Enable Cortex-A53-specific code paths by default if core is detected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-07 01:51 PST by Akos Kiss
Modified: 2014-11-10 02:38 PST (History)
7 users (show)

See Also:


Attachments
Proposed patch. (1.50 KB, patch)
2014-11-07 01:54 PST, Akos Kiss
no flags Details | Formatted Diff | Diff
Proposed patch, v2 (2.21 KB, patch)
2014-11-07 06:36 PST, Akos Kiss
no flags Details | Formatted Diff | Diff
Proposed patch, v3 (2.21 KB, patch)
2014-11-07 06:42 PST, Akos Kiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Akos Kiss 2014-11-07 01:51:50 PST
This is a follow-up to https://bugs.webkit.org/show_bug.cgi?id=138315 . It would be good if the build system would detect that we are on a Cortex-A53 and enable specific code paths by default, with no need for --cmakeargs="-DWTF_CPU_ARM64_CORTEXA53=ON" command line arguments (which are easy to forget or misspell).
Comment 1 Akos Kiss 2014-11-07 01:54:15 PST
Created attachment 241168 [details]
Proposed patch.
Comment 2 Csaba Osztrogonác 2014-11-07 02:28:56 PST
Comment on attachment 241168 [details]
Proposed patch.

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

> Source/cmake/OptionsCommon.cmake:36
> +    if (CPUINFO MATCHES "0xd03")

I would restrict matching "0xd03" only in "CPU part" line to avoid accidental matches.

And could you possibly add a comment to the ARM reference?
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500f/BABFEABI.html

> Source/cmake/OptionsCommon.cmake:37
> +        set(WTF_CPU_ARM64_CORTEXA53_INITIALVALUE ON)

Why not set(WTF_CPU_ARM64_CORTEXA53 1) and add_definitions(-DWTF_CPU_ARM64_CORTEXA53=1) ?
In this case we wouldn't need WTF_CPU_ARM64_CORTEXA53_INITIALVALUE and the option.
Comment 3 Akos Kiss 2014-11-07 06:36:51 PST
Created attachment 241176 [details]
Proposed patch, v2

The updated patch now checks for 0xd03 only in the CPU part line, as requested. Also, comments are added. 

Moreover, the patch executes the check multiple times, once for each available core, because the /proc/cpuinfo API is somewhat strange on ARM64 and the result depends on the core the query is executed on. This might conceal the presence of an A53 core in a heterogeneous setup.

Option and _INITIALVALUE are used to allow explicit setting of WTF_CPU_ARM64_CORTEXA53 from command line, independently from the result of the detection.
Comment 4 WebKit Commit Bot 2014-11-07 06:39:47 PST
Attachment 241176 [details] did not pass style-queue:


ERROR: Source/cmake/OptionsCommon.cmake:46:  No space after "("  [whitespace/parentheses] [5]
ERROR: Source/cmake/OptionsCommon.cmake:46:  No space before ")"  [whitespace/parentheses] [5]
ERROR: Source/cmake/OptionsCommon.cmake:46:  One space between command "endforeach" and its parentheses, should be "endforeach ("  [whitespace/parentheses] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Akos Kiss 2014-11-07 06:42:53 PST
Created attachment 241178 [details]
Proposed patch, v3

One misplaced space. My bad. Sorry for the noise.
Comment 6 Csaba Osztrogonác 2014-11-10 02:01:37 PST
Comment on attachment 241178 [details]
Proposed patch, v3

r=me
Comment 7 WebKit Commit Bot 2014-11-10 02:38:12 PST
Comment on attachment 241178 [details]
Proposed patch, v3

Clearing flags on attachment: 241178

Committed r175804: <http://trac.webkit.org/changeset/175804>
Comment 8 WebKit Commit Bot 2014-11-10 02:38:18 PST
All reviewed patches have been landed.  Closing bug.