90198 – Port DFG JIT to traditional ARM

Bug 90198 - Port DFG JIT to traditional ARM

Summary: Port DFG JIT to traditional ARM

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:

Depends on:	90656 160288
Blocks:	90644 91238
	Show dependency tree / graph

Reported:	2012-06-28 12:52 PDT by Zoltan Herczeg
Modified:	2016-07-28 04:28 PDT (History)
CC List:	4 users (show)

See Also:

Attachments
first draft patch (68.26 KB, patch) 2012-06-28 12:54 PDT, Zoltan Herczeg	no flags	Details \| Formatted Diff \| Diff
second draft patch (76.71 KB, patch) 2012-07-02 03:41 PDT, Zoltan Herczeg	no flags	Details \| Formatted Diff \| Diff
working patch (77.03 KB, patch) 2012-07-02 08:11 PDT, Zoltan Herczeg	no flags	Details \| Formatted Diff \| Diff
patch (85.61 KB, patch) 2012-07-03 05:26 PDT, Zoltan Herczeg	fpizlo: review+	Details \| Formatted Diff \| Diff
latest patch (84.16 KB, patch) 2012-07-04 02:39 PDT, Zoltan Herczeg	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

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

Description Zoltan Herczeg 2012-06-28 12:52:15 PDT

Let's do it.

Comment 1 Zoltan Herczeg 2012-06-28 12:54:30 PDT

Created attachment 149988 [details]
first draft patch

There is some progress on this porting work. Most of the JSCore tests are running now, but some runtime assertions are hit during the test.

Comment 2 WebKit Review Bot 2012-06-28 12:57:36 PDT

Attachment 149988 [details] did not pass style-queue:

Source/JavaScriptCore/dfg/DFGOperations.cpp:146:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:157:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:178:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:189:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1384:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1393:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.cpp:295:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/ARMAssembler.cpp:331:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:430:  vmov_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:465:  vabs_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:470:  vneg_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:485:  dtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:490:  dtr_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:495:  dtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:500:  dtr_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:505:  dtrh_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:510:  dtrh_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:515:  dtrh_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:520:  dtrh_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:525:  fdtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:532:  fdtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:561:  vmov_vfp64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:567:  vmov_arm64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:573:  vmov_vfp32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:579:  vmov_arm32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:597:  vcvt_u32_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:603:  vcvt_f64_f32_r is incorrectly named. Don't use underscores in your identifieFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/assembler/ARMAssembl..." exit_code: 1
r names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:609:  vcvt_f32_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:893:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:910:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:911:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:912:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:913:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:914:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:915:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 35 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 3 Zoltan Herczeg 2012-07-02 03:41:34 PDT

Created attachment 150392 [details]
second draft patch

3 regressions in jscore tests.

Comment 4 WebKit Review Bot 2012-07-02 03:43:33 PDT

Attachment 150392 [details] did not pass style-queue:

Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1205:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:146:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:157:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:178:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:189:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1382:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1391:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.cpp:295:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/ARMAssembler.cpp:331:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:433:  vmov_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:468:  vabs_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:473:  vneg_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:488:  dtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:493:  dtr_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:498:  dtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:503:  dtr_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:508:  dtrh_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:513:  dtrh_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:518:  dtrh_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:523:  dtrh_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:528:  fdtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:535:  fdtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:564:  vmov_vfp64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:570:  vmov_arm64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:576:  vmov_vfp32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:582:  vmov_arm32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:600:  vcvt_u32_f64_r is incorrectly named. DoFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/assembler/ARMAssembl..." exit_code: 1
n't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:606:  vcvt_f64_f32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:612:  vcvt_f32_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:932:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:949:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:950:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:951:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:952:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:953:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:954:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 36 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 5 Zoltan Herczeg 2012-07-02 08:11:14 PDT

Created attachment 150423 [details]
working patch

The patch is working now.

SunSpider:

With DFG: 1910.2ms
Without DFG 2062.6ms

ChangeLog is missing but I think the patch is ready for a first round of review.

Comment 6 WebKit Review Bot 2012-07-02 08:13:14 PDT

Attachment 150423 [details] did not pass style-queue:

Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1205:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:146:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:157:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:178:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:189:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1382:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1391:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.cpp:295:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/ARMAssembler.cpp:331:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:433:  vmov_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:468:  vabs_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:473:  vneg_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:488:  dtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:493:  dtr_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:498:  dtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:503:  dtr_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:508:  dtrh_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:513:  dtrh_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:518:  dtrh_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:523:  dtrh_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:528:  fdtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:535:  fdtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:564:  vmov_vfp64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:570:  vmov_arm64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:576:  vmov_vfp32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:582:  vmov_arm32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:600:  vcvt_u32_f64_r is incorrectly named. DoFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/assembler/ARMAssembl..." exit_code: 1
n't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:606:  vcvt_f64_f32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:612:  vcvt_f32_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:932:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:949:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:950:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:951:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:952:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:953:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:954:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 36 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 7 Zoltan Herczeg 2012-07-03 05:26:49 PDT

Created attachment 150590 [details]
patch

Some minor things were refactored and a changelog is added.

Comment 8 WebKit Review Bot 2012-07-03 05:29:27 PDT

Attachment 150590 [details] did not pass style-queue:

Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1195:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:146:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:157:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:178:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:189:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1382:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1391:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.cpp:295:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/ARMAssembler.cpp:331:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:433:  vmov_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:468:  vabs_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:473:  vneg_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:488:  dtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:493:  dtr_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:498:  dtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:503:  dtr_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:508:  dtrh_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:513:  dtrh_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:518:  dtrh_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:523:  dtrh_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:528:  fdtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:535:  fdtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:564:  vmov_vfp64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:570:  vmov_arm64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:576:  vmov_vfp32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:582:  vmov_arm32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:600:  vcvt_u32_f64_r is incorrectly named. DoFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
n't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:606:  vcvt_f64_f32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:612:  vcvt_f32_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:933:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 30 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 9 Filip Pizlo 2012-07-03 08:57:11 PDT

Comment on attachment 150590 [details]
patch

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

R=me.  Please measure V8 and Kraken though.  You should see big speed-ups there; if you don't then there's something wrong!

> Source/JavaScriptCore/ChangeLog:13
> +        Sunspider is improved by 8%.

Affect on V8 performance?  Kraken?

Comment 10 Gabor Loki 2012-07-04 01:20:36 PDT

Comment on attachment 150590 [details]
patch

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

>> Source/JavaScriptCore/assembler/ARMAssembler.cpp:295
>> +    if (offset == 0) {
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

You should refine this and other style errors below. ;)

> Source/JavaScriptCore/assembler/ARMAssembler.h:-158
> -#if WTF_ARM_ARCH_AT_LEAST(5) || defined(__ARM_ARCH_4T__)

Please mention in the ChangeLog that the support of ARMv4 and below is removed.

Additionally you should check WTF_ARM_ARCH_AT_LEAST(4) and similar constructs and modify them in Platform.h and other files.

> Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:269
> +#if CPU(ARM_THUMB2)
>          m_assembler.vmov(payloadGPR, tagGPR, fpr);
> +#else
> +        m_assembler.vmov_arm64_r(payloadGPR, tagGPR, fpr);
> +#endif

I suggest to define a common name or a wrapper function reducing #ifdef burden for the same logic.

Apart from these, I really like your patch. ;)

Comment 11 Filip Pizlo 2012-07-04 01:24:52 PDT

(In reply to comment #10)
> (From update of attachment 150590 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150590&action=review
> 
> >> Source/JavaScriptCore/assembler/ARMAssembler.cpp:295
> >> +    if (offset == 0) {
> > 
> > Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> 
> You should refine this and other style errors below. ;)
> 
> > Source/JavaScriptCore/assembler/ARMAssembler.h:-158
> > -#if WTF_ARM_ARCH_AT_LEAST(5) || defined(__ARM_ARCH_4T__)
> 
> Please mention in the ChangeLog that the support of ARMv4 and below is removed.
> 
> Additionally you should check WTF_ARM_ARCH_AT_LEAST(4) and similar constructs and modify them in Platform.h and other files.
> 
> > Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:269
> > +#if CPU(ARM_THUMB2)
> >          m_assembler.vmov(payloadGPR, tagGPR, fpr);
> > +#else
> > +        m_assembler.vmov_arm64_r(payloadGPR, tagGPR, fpr);
> > +#endif
> 
> I suggest to define a common name or a wrapper function reducing #ifdef burden for the same logic.
> 
> Apart from these, I really like your patch. ;)

+1

I like the idea of wrapping the "m_assembler.vmov_blah" in something nicer.  Perhaps you can add a method in DFGAssemblyHelpers.h like:

void transferReturnValueToReturnValueFPR()
{
#if thumb2
     the thumb2 thingy
#else if arm
     the arm thingy
#else
     do thing
#endif
}

I think that would be useful in other places in the future, particularly since there are code paths in the DFG that make calls in a manner that bypasses the helper methods for a variety of reasons.  Currently none of those deal with floating point returns, but that might change.

Comment 12 Zoltan Herczeg 2012-07-04 01:33:12 PDT

I finished the V8 measurements.

Without DFG: 15372.2ms
With DFG: 8010.5ms

So there is a nice progression here.

Comment 13 Filip Pizlo 2012-07-04 01:33:41 PDT

(In reply to comment #12)
> I finished the V8 measurements.
> 
> Without DFG: 15372.2ms
> With DFG: 8010.5ms
> 
> So there is a nice progression here.

Fantastic!

Comment 14 Zoltan Herczeg 2012-07-04 01:42:48 PDT

> Please mention in the ChangeLog that the support of ARMv4 and below is removed.

It is already there :)

Ok I will do the necessary style changes. I think further refactoring could be done in followup patches, but this patch is already big enough, so I focus on the DFG related changes.

Comment 15 Zoltan Herczeg 2012-07-04 02:39:27 PDT

Created attachment 150746 [details]
latest patch

Comment 16 WebKit Review Bot 2012-07-04 02:41:32 PDT

Attachment 150746 [details] did not pass style-queue:

Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1195:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:146:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:157:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:178:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:189:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1382:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:1391:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/assembler/ARMAssembler.h:433:  vmov_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:468:  vabs_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:473:  vneg_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:488:  dtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:493:  dtr_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:498:  dtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:503:  dtr_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:508:  dtrh_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:513:  dtrh_ur is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:518:  dtrh_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:523:  dtrh_dr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:528:  fdtr_u is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:535:  fdtr_d is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:564:  vmov_vfp64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:570:  vmov_arm64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:576:  vmov_vfp32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:582:  vmov_arm32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:600:  vcvt_u32_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:606:  vcvt_f64_f32_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/ARMAssembler.h:612:  vcvt_f32_f64_r is incorrectly named. Don't use underscores in your identifier names.  [readability/namingFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
] [4]
Total errors found: 27 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 17 Zoltan Herczeg 2012-07-04 02:45:23 PDT

> void transferReturnValueToReturnValueFPR()
> {
> #if thumb2
>      the thumb2 thingy
> #else if arm
>      the arm thingy
> #else
>      do thing
> #endif
> }

I am not sure I understand this, but I added two vmov-s to the traditional assemblers, which wraps the "arm thingy" and this way I could remove all ifdefs from the code.

Comment 18 Filip Pizlo 2012-07-04 12:42:34 PDT

(In reply to comment #17)
> > void transferReturnValueToReturnValueFPR()
> > {
> > #if thumb2
> >      the thumb2 thingy
> > #else if arm
> >      the arm thingy
> > #else
> >      do thing
> > #endif
> > }
> 
> I am not sure I understand this, but I added two vmov-s to the traditional assemblers, which wraps the "arm thingy" and this way I could remove all ifdefs from the code.

Argh, sorry, I got confused by context.  Never mind.

Comment 19 Zoltan Herczeg 2012-07-05 00:05:33 PDT

Landed as http://trac.webkit.org/changeset/121885

Comment 20 Filip Pizlo 2012-07-05 16:08:16 PDT

FYI, this appears to have broken the ARMv7 port.  I'll fix it.

Comment 21 Filip Pizlo 2012-07-05 16:31:45 PDT

(In reply to comment #20)
> FYI, this appears to have broken the ARMv7 port.  I'll fix it.

Fixed in http://trac.webkit.org/changeset/121927

Comment 22 Zoltan Herczeg 2012-07-06 00:20:40 PDT

> Fixed in http://trac.webkit.org/changeset/121927

This macro should only used by traditional arm, and as far as I see all of its uses are guarded by CPU(ARM_TRADITIONAL). How does it slip out?

Comment 23 Filip Pizlo 2012-07-06 00:25:19 PDT

(In reply to comment #22)
> > Fixed in http://trac.webkit.org/changeset/121927
> 
> This macro should only used by traditional arm, and as far as I see all of its uses are guarded by CPU(ARM_TRADITIONAL). How does it slip out?

Ah, interesting.

Around line 1089 in JITStubs.cpp:

#if CPU(ARM_THUMB2) && COMPILER(GCC)

#define DEFINE_STUB_FUNCTION(rtype, op) \
    extern "C" { \
        rtype JITStubThunked_##op(STUB_ARGS_DECLARATION); \
    }; \
    asm ( \
        ".text" "\n" \
        ".align 2" "\n" \
        ".globl " SYMBOL_STRING(cti_##op) "\n" \
        HIDE_SYMBOL(cti_##op) "\n"             \
        INLINE_ARM_FUNCTION(cti_##op) \
        SYMBOL_STRING(cti_##op) ":" "\n" \
        "str lr, [sp, #" STRINGIZE_VALUE_OF(THUNK_RETURN_ADDRESS_OFFSET) "]" "\n" \
        "bl " SYMBOL_STRING(JITStubThunked_##op) "\n" \
        "ldr lr, [sp, #" STRINGIZE_VALUE_OF(THUNK_RETURN_ADDRESS_OFFSET) "]" "\n" \
        "bx lr" "\n" \
        ); \
    rtype JITStubThunked_##op(STUB_ARGS_DECLARATION) \

Maybe you didn't mean to use INLINE_ARM_FUNCTION there?

Comment 24 Zoltan Herczeg 2012-07-06 00:50:24 PDT

That is my mistake. I accidentaly replaced that macro as well. I will revert it.

-        ".thumb" "\n" \
-        ".thumb_func " THUMB_FUNC_PARAM(cti_##op) "\n" \
+        INLINE_ARM_FUNCTION(cti_##op) \