Bug 151519 – Use the JITAddGenerator snippet in the FTL.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/23637757>

[Mark Lam]

Created attachment 266012 [details]
patch 1.

This patch passes the JSC tests on x86_64.  Still need to benchmark it, test it on ARM64, and tune the IC size for the ValueAdd IC.  Other than that, the code is in what I expect to be the final form.

Feedback is welcomed.  Thanks.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:152:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:153:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:154:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:155:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:156:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:159:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:160:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:161:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:162:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:341:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:344:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:38:  Do not use 'using namespace DFG;'.  [build/using_namespace] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:41:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:42:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:53:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/ftl/FTLCompileBinaryOp.cpp:193:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ftl/FTLCompileBinaryOp.cpp:194:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 20 in 18 files


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

[Mark Lam]

Created attachment 266015 [details]
patch 2: fixed valid style issues.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:152:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:153:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:154:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:155:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:156:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:159:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:160:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:161:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:162:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:39:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:51:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:51:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 12 in 18 files


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

[Mark Lam]

Created attachment 266189 [details]
x86_64 benchmark result.

Benchmarks show that perf is neutral.  Any of the "definitely" faster/slower results are just noise, and do not reproduce when I re-run them.

I tried adding an ftl-object-add.js test to specifically test the effects of polymorphic operands on FTL add performance.  The result was no significant change.  This is because the FTL (and DFG) had already previously supported polymorphic adds (ValueAdd) with an unconditional call to its slow path function.

This patch introduces the ability to handle numeric types without going to the slow path, which evidently does not contribute significant difference in perf.  We should move forward with this patch anyway because:
1. it keeps the op_add implementation consistent with the other operators.
2. it introduces the common BinaryOpDescriptor which make it easier to add future binary operators (implemented as snippets) to the FTL.

[Mark Lam]

Comment on attachment 266015 [details]
patch 2: fixed valid style issues.

I need to rebase this patch in light of recent commits.  I will also split this into 2 patches: 1 to refactor the FTL code for binary snippets (new bug), 1 for the FTL ValueAdd snippet (this bug).

[Mark Lam]

Created attachment 266379 [details]
rebased patch.

Still waiting for tests of IC sizes to finish running.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:52:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:52:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 9 files


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

[Geoffrey Garen]

Comment on attachment 266379 [details]
rebased patch.

r=me

[Mark Lam]

Created attachment 266414 [details]
patch 4: plus 2 bug fixes.

Added 2 bug fixes:
1. In lower() in FTLLowerDFGToLLVM.cpp, I added the case for ValueAdd where it needs to account for a potential need of a spill location on the stack.
2. In compileValueAdd(), I moved the increment m_stackmapIDs a little lower in the function to the point where we are committed to allocated a new patchpoint.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:52:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:52:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 9 files


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

[Geoffrey Garen]

Comment on attachment 266414 [details]
patch 4: plus 2 bug fixes.

r=me

[Mark Lam]

Created attachment 266415 [details]
patch 5: preserve original compileValueAdd() when #if FTL_USES_B3.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:52:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:52:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 9 files


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

[Mark Lam]

Created attachment 266454 [details]
patch 6: added back CRASH() for B3 in the same way that ArithSub does it.

[Mark Lam]

Comment on attachment 266454 [details]
patch 6: added back CRASH() for B3 in the same way that ArithSub does it.

Wrong patch.

[Mark Lam]

Created attachment 266455 [details]
patch 6: added back CRASH() for B3 in the same way that ArithSub does it (again).

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:52:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptorInlines.h:52:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 2 in 9 files


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

[Geoffrey Garen]

Comment on attachment 266455 [details]
patch 6: added back CRASH() for B3 in the same way that ArithSub does it (again).

r=me

[Mark Lam]

Thanks for the review.  Landed in r192950: <http://trac.webkit.org/r192950>.