Bug 109371

Summary:

DFG should not change its mind about what type speculations a node does, by encoding the checks in the NodeType, UseKind, and ArrayMode

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, ggaren, gyuyoung.kim, jberlin, mark.lam, mhahnenberg, msaboff, oliver, rakuco, sam, webkit.review.bot, zarvai

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

110072, 110590, 110756

Bug Blocks:

109388, 109389, 109393

Attachments:

Description	Flags
work in progress	none
work in progress	none
work in progress	none
fixup phase is done, I think	none
more	none
work in progress	none
work in progress	none
more things	none
more	none
more	none
more	none
DFGSpeculativeJIT64 is done	none
it compiles (in 64-bit)	none
it runs simple things	none
it runs the most important program	none
runs SunSpider, V8, Kraken	none
half-way done with 32-bit, and all LayoutTests pass in 64-bit	none
32-bit is written	none
the patch	none
the patch	oliver: review+

Filip Pizlo

Reported 2013-02-09 22:22:17 PST

Currently the type guards that a node emits are usually determined by the predictions of the children. But children can change as a result of CSE and CFG simplification. Hence nodes can change behavior, and I'm not convinced that this is OK. Logically, it probably is, since type predictions are likely to get more, rather than less, specific - which then leads to more, rather than less, specific guards. But I don't want to have to think about this. It's unlikely that this behavior is profitable, and I worry that it leads to badness. So, it's probably better to change the behavior so that the type guards that a node uses are locked in after prediction propagation and thereafter they don't change.

Attachments
work in progress (19.66 KB, patch) 2013-02-09 22:23 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (27.86 KB, patch) 2013-02-10 13:54 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (54.75 KB, patch) 2013-02-12 22:15 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
fixup phase is done, I think (57.13 KB, patch) 2013-02-16 01:01 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
more (77.24 KB, patch) 2013-02-17 01:40 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (105.68 KB, patch) 2013-02-17 15:48 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (126.89 KB, patch) 2013-02-17 18:54 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
more things (146.10 KB, patch) 2013-02-18 00:34 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
more (189.36 KB, patch) 2013-02-18 12:58 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
more (215.20 KB, patch) 2013-02-18 15:51 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
more (235.47 KB, patch) 2013-02-18 21:59 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
DFGSpeculativeJIT64 is done (252.88 KB, patch) 2013-02-18 23:36 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
it compiles (in 64-bit) (286.71 KB, patch) 2013-02-19 15:10 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
it runs simple things (288.99 KB, patch) 2013-02-19 15:49 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
it runs the most important program (299.77 KB, patch) 2013-02-19 19:01 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
runs SunSpider, V8, Kraken (302.20 KB, patch) 2013-02-19 21:01 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
half-way done with 32-bit, and all LayoutTests pass in 64-bit (358.07 KB, patch) 2013-02-20 14:34 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
32-bit is written (375.47 KB, patch) 2013-02-20 16:30 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (379.81 KB, patch) 2013-02-20 20:57 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (379.86 KB, patch) 2013-02-20 21:08 PST, Filip Pizlo	oliver: review+	Details Formatted Diff Diff
Show Obsolete (19) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2013-02-09 22:23:47 PST

Created attachment 187463 [details] work in progress This is going to be rough.

Filip Pizlo

Comment 2 2013-02-10 13:54:22 PST

Created attachment 187494 [details] work in progress Wrote more code, found more bugs.

Filip Pizlo

Comment 3 2013-02-12 22:15:30 PST

Created attachment 188010 [details] work in progress

Filip Pizlo

Comment 4 2013-02-16 01:01:54 PST

Created attachment 188699 [details] fixup phase is done, I think Now I just have to redo the rest of the compiler.

Filip Pizlo

Comment 5 2013-02-17 01:40:56 PST

Created attachment 188758 [details] more

Filip Pizlo

Comment 6 2013-02-17 15:48:18 PST

Created attachment 188780 [details] work in progress I think Fixup and CFA are done.

Filip Pizlo

Comment 7 2013-02-17 18:54:17 PST

Created attachment 188788 [details] work in progress Started refactoring the backend.

Filip Pizlo

Comment 8 2013-02-18 00:34:38 PST

Created attachment 188807 [details] more things

Filip Pizlo

Comment 9 2013-02-18 12:58:06 PST

Created attachment 188932 [details] more

Filip Pizlo

Comment 10 2013-02-18 15:51:12 PST

Created attachment 188955 [details] more

Filip Pizlo

Comment 11 2013-02-18 21:59:47 PST

Created attachment 188986 [details] more

Filip Pizlo

Comment 12 2013-02-18 23:36:28 PST

Created attachment 189003 [details] DFGSpeculativeJIT64 is done

Filip Pizlo

Comment 13 2013-02-19 15:10:14 PST

Created attachment 189176 [details] it compiles (in 64-bit)

Filip Pizlo

Comment 14 2013-02-19 15:49:51 PST

Created attachment 189188 [details] it runs simple things

Filip Pizlo

Comment 15 2013-02-19 15:51:33 PST

Sample program: function foo(a, b) { return a + 42 + b.f; } for (var i = 0; i < 100; ++i) print(foo(i, {f:i})); Sample disassembly with IR under the new locked-in type speculation regime: Generated DFG JIT code for foo#CDdMUV:[0x7fca3b00f800->0x10e2dfd70, DFGFunctionCall], instruction count = 22: Optimized with execution counter = 1005.000000/1121.000000, 5 Source: function foo(a, b) { return a + 42 + b.f; } Code at [0x25f415a014e0, 0x25f415a016a7): 0x25f415a014e0: pop %rcx 0x25f415a014e1: mov %rcx, -0x10(%r13) 0x25f415a014e5: mov $0x7fca3b00f800, %r11 0x25f415a014ef: mov %r11, -0x8(%r13) 0x25f415a014f3: lea 0x18(%r13), %rdx 0x25f415a014f7: mov $0x7fca3a002630, %r11 0x25f415a01501: cmp %rdx, (%r11) 0x25f415a01504: jb 0x25f415a0159b 0x25f415a0150a: cmp %r14, -0x40(%r13) 0x25f415a0150e: jb 0x25f415a01602 0x25f415a01514: test %r15, -0x48(%r13) 0x25f415a01518: jnz 0x25f415a01623 Block #0 (bc#0): (OSR target) Predecessors: Dominated by: #0 Dominates: #0 Phi Nodes: 0x25f415a0151e: test $0x7, %r13b 0x25f415a01522: jz 0x25f415a01529 0x25f415a01528: int3 1: < 2:-> SetArgument(arg1(B<Int32>), bc#0) predicting Int 2: < 2:-> SetArgument(arg2(C<Final>), bc#0) predicting Final 16: < 3:0> GetLocal(@2, JS|PureInt, arg2(C<Final>), bc#0) predicting Final 0x25f415a01529: mov -0x48(%r13), %rax 17: <!0:-> CheckStructure(Cell:@16<Final>, MustGen|CanExit, struct(0x10cdfc4f0: NonArray), bc#0) 0x25f415a0152d: mov $0x10cdfc4f0, %r11 0x25f415a01537: cmp %r11, (%rax) 0x25f415a0153a: jnz 0x25f415a01644 3: < 1:1> GetLocal(@1, JS|UseAsOther, arg1(B<Int32>), bc#1) predicting Int 0x25f415a01540: mov -0x40(%r13), %edx 4: < 1:2> JSConstant(JS|UseAsOther, $0 = Int32: 42, bc#1) 5: <!1:2> ValueAdd(Int32:@3<Int32>, Int32:@4<Int32>, JS|MustGen|MightClobber|UseAsOther|CanExit, bc#1) 0x25f415a01544: mov $0xc9, %ecx 0x25f415a01549: xor $0xe3, %ecx 0x25f415a0154f: add %edx, %ecx 0x25f415a01551: jo 0x25f415a01665 0x25f415a01557: mov $0xffffffff, %r11 0x25f415a01561: cmp %r11, %rcx 0x25f415a01564: jbe 0x25f415a0156b 0x25f415a0156a: int3 6: skipped < 0:-> SetLocal(@5<Int32>, CanExit, r0(D), bc#1) 8: <!0:-> Phantom(Cell:@16<Final>, MustGen, bc#6) 9: < 1:0> GetByOffset(KnownCell:@16<Final>, JS|UseAsOther, id0{f}, 2, bc#6) predicting Int 0x25f415a0156b: mov 0x10(%rax), %rdx 10: skipped < 0:-> SetLocal(@9<Int32>, CanExit, r1(E), bc#6) 11: <!1:0> ValueAdd(Int32:@5<Int32>, Int32:@9<Int32>, JS|MustGen|MightClobber|UseAsOther|CanExit, bc#15) 0x25f415a0156f: add %edx, %ecx 0x25f415a01571: jo 0x25f415a01686 0x25f415a01577: mov $0xffffffff, %r11 0x25f415a01581: cmp %r11, %rcx 0x25f415a01584: jbe 0x25f415a0158b 0x25f415a0158a: int3 12: skipped < 0:-> SetLocal(@11<Int32>, CanExit, r0(F), bc#15) 13: <!0:-> Flush(@2, MustGen, arg2(C<Final>), bc#20) predicting Final 14: <!0:-> Flush(@1, MustGen, arg1(B<Int32>), bc#20) predicting Int 15: <!0:-> Return(@11<Int32>, MustGen, bc#20) 0x25f415a0158b: or %r14, %rcx 0x25f415a0158e: mov %rcx, %rax 0x25f415a01591: mov -0x10(%r13), %rdx 0x25f415a01595: mov -0x28(%r13), %r13 0x25f415a01599: push %rdx 0x25f415a0159a: ret (End Of Main Path) 0x25f415a0159b: mov %rsp, %rdi 0x25f415a0159e: mov %r13, 0x58(%rsp) 0x25f415a015a3: mov $0x0, -0x2c(%r13) 0x25f415a015ab: mov $0x10ba08ee0, %r11 0x25f415a015b5: call %r11 0x25f415a015b8: jmp 0x25f415a0150a 0x25f415a015bd: pop %rcx 0x25f415a015be: mov %rcx, -0x10(%r13) 0x25f415a015c2: mov $0x7fca3b00f800, %r11 0x25f415a015cc: mov %r11, -0x8(%r13) 0x25f415a015d0: mov -0x30(%r13), %edx 0x25f415a015d4: cmp $0x3, %edx 0x25f415a015d7: jae 0x25f415a014f3 0x25f415a015dd: mov %rsp, %rdi 0x25f415a015e0: mov %r13, 0x58(%rsp) 0x25f415a015e5: mov $0x1, -0x2c(%r13) 0x25f415a015ed: mov $0x10ba0df30, %r11 0x25f415a015f7: call %r11 0x25f415a015fa: mov %rax, %r13 0x25f415a015fd: jmp 0x25f415a014f3 0x25f415a01602: test $0x7, %r13b 0x25f415a01606: jz 0x25f415a0160d 0x25f415a0160c: int3 0x25f415a0160d: mov $0x7fca39814288, %r11 0x25f415a01617: mov $0x0, (%r11) 0x25f415a0161e: jmp 0x25f415a016c0 0x25f415a01623: test $0x7, %r13b 0x25f415a01627: jz 0x25f415a0162e 0x25f415a0162d: int3 0x25f415a0162e: mov $0x7fca39814288, %r11 0x25f415a01638: mov $0x1, (%r11) 0x25f415a0163f: jmp 0x25f415a016c0 0x25f415a01644: test $0x7, %r13b 0x25f415a01648: jz 0x25f415a0164f 0x25f415a0164e: int3 0x25f415a0164f: mov $0x7fca39814288, %r11 0x25f415a01659: mov $0x2, (%r11) 0x25f415a01660: jmp 0x25f415a016c0 0x25f415a01665: test $0x7, %r13b 0x25f415a01669: jz 0x25f415a01670 0x25f415a0166f: int3 0x25f415a01670: mov $0x7fca39814288, %r11 0x25f415a0167a: mov $0x3, (%r11) 0x25f415a01681: jmp 0x25f415a016c0 0x25f415a01686: test $0x7, %r13b 0x25f415a0168a: jz 0x25f415a01691 0x25f415a01690: int3 0x25f415a01691: mov $0x7fca39814288, %r11 0x25f415a0169b: mov $0x4, (%r11) 0x25f415a016a2: jmp 0x25f415a016c0

Filip Pizlo

Comment 16 2013-02-19 16:00:39 PST

And here's the code from a release build: Generated DFG JIT code for foo#CDdMUV:[0x105453000->0x105ecfd70, DFGFunctionCall], instruction count = 22: Optimized with execution counter = 1005.000000/1121.000000, 5 Source: function foo(a, b) { return a + 42 + b.f; } Code at [0x4c16e48014e0, 0x4c16e4801636): 0x4c16e48014e0: pop %rcx 0x4c16e48014e1: mov %rcx, -0x10(%r13) 0x4c16e48014e5: mov $0x105453000, %r11 0x4c16e48014ef: mov %r11, -0x8(%r13) 0x4c16e48014f3: lea 0x18(%r13), %rdx 0x4c16e48014f7: mov $0x105408c90, %r11 0x4c16e4801501: cmp %rdx, (%r11) 0x4c16e4801504: jb 0x4c16e4801561 0x4c16e480150a: cmp %r14, -0x40(%r13) 0x4c16e480150e: jb 0x4c16e48015c8 0x4c16e4801514: test %r15, -0x48(%r13) 0x4c16e4801518: jnz 0x4c16e48015de Block #0 (bc#0): (OSR target) Predecessors: Dominated by: #0 Dominates: #0 Phi Nodes: 1: < 2:-> SetArgument(arg1(B<Int32>), bc#0) predicting Int 2: < 2:-> SetArgument(arg2(C<Final>), bc#0) predicting Final 16: < 3:0> GetLocal(@2, JS|PureInt, arg2(C<Final>), bc#0) predicting Final 0x4c16e480151e: mov -0x48(%r13), %rax 17: <!0:-> CheckStructure(Cell:@16<Final>, MustGen|CanExit, struct(0x105dec5d0: NonArray), bc#0) 0x4c16e4801522: mov $0x105dec5d0, %r11 0x4c16e480152c: cmp %r11, (%rax) 0x4c16e480152f: jnz 0x4c16e48015f4 3: < 1:1> GetLocal(@1, JS|UseAsOther, arg1(B<Int32>), bc#1) predicting Int 0x4c16e4801535: mov -0x40(%r13), %edx 4: < 1:2> JSConstant(JS|UseAsOther, $0 = Int32: 42, bc#1) 5: <!1:2> ValueAdd(Int32:@3<Int32>, Int32:@4<Int32>, JS|MustGen|MightClobber|UseAsOther|CanExit, bc#1) 0x4c16e4801539: mov %rdx, %rcx 0x4c16e480153c: add $0x2a, %ecx 0x4c16e480153f: jo 0x4c16e480160a 6: skipped < 0:-> SetLocal(@5<Int32>, CanExit, r0(D), bc#1) 8: <!0:-> Phantom(Cell:@16<Final>, MustGen, bc#6) 9: < 1:0> GetByOffset(KnownCell:@16<Final>, JS|UseAsOther, id0{f}, 2, bc#6) predicting Int 0x4c16e4801545: mov 0x10(%rax), %rdx 10: skipped < 0:-> SetLocal(@9<Int32>, CanExit, r1(E), bc#6) 11: <!1:0> ValueAdd(Int32:@5<Int32>, Int32:@9<Int32>, JS|MustGen|MightClobber|UseAsOther|CanExit, bc#15) 0x4c16e4801549: add %edx, %ecx 0x4c16e480154b: jo 0x4c16e4801620 12: skipped < 0:-> SetLocal(@11<Int32>, CanExit, r0(F), bc#15) 13: <!0:-> Flush(@2, MustGen, arg2(C<Final>), bc#20) predicting Final 14: <!0:-> Flush(@1, MustGen, arg1(B<Int32>), bc#20) predicting Int 15: <!0:-> Return(@11<Int32>, MustGen, bc#20) 0x4c16e4801551: or %r14, %rcx 0x4c16e4801554: mov %rcx, %rax 0x4c16e4801557: mov -0x10(%r13), %rdx 0x4c16e480155b: mov -0x28(%r13), %r13 0x4c16e480155f: push %rdx 0x4c16e4801560: ret (End Of Main Path) 0x4c16e4801561: mov %rsp, %rdi 0x4c16e4801564: mov %r13, 0x58(%rsp) 0x4c16e4801569: mov $0x0, -0x2c(%r13) 0x4c16e4801571: mov $0x103f719b0, %r11 0x4c16e480157b: call %r11 0x4c16e480157e: jmp 0x4c16e480150a 0x4c16e4801583: pop %rcx 0x4c16e4801584: mov %rcx, -0x10(%r13) 0x4c16e4801588: mov $0x105453000, %r11 0x4c16e4801592: mov %r11, -0x8(%r13) 0x4c16e4801596: mov -0x30(%r13), %edx 0x4c16e480159a: cmp $0x3, %edx 0x4c16e480159d: jae 0x4c16e48014f3 0x4c16e48015a3: mov %rsp, %rdi 0x4c16e48015a6: mov %r13, 0x58(%rsp) 0x4c16e48015ab: mov $0x1, -0x2c(%r13) 0x4c16e48015b3: mov $0x103f74470, %r11 0x4c16e48015bd: call %r11 0x4c16e48015c0: mov %rax, %r13 0x4c16e48015c3: jmp 0x4c16e48014f3 0x4c16e48015c8: mov $0x1054169d0, %r11 0x4c16e48015d2: mov $0x0, (%r11) 0x4c16e48015d9: jmp 0x4c16e4801640 0x4c16e48015de: mov $0x1054169d0, %r11 0x4c16e48015e8: mov $0x1, (%r11) 0x4c16e48015ef: jmp 0x4c16e4801640 0x4c16e48015f4: mov $0x1054169d0, %r11 0x4c16e48015fe: mov $0x2, (%r11) 0x4c16e4801605: jmp 0x4c16e4801640 0x4c16e480160a: mov $0x1054169d0, %r11 0x4c16e4801614: mov $0x3, (%r11) 0x4c16e480161b: jmp 0x4c16e4801640 0x4c16e4801620: mov $0x1054169d0, %r11 0x4c16e480162a: mov $0x4, (%r11) 0x4c16e4801631: jmp 0x4c16e4801640

Filip Pizlo

Comment 17 2013-02-19 19:01:27 PST

Created attachment 189222 [details] it runs the most important program I'm referring to 3d-cube, of course.

Filip Pizlo

Comment 18 2013-02-19 21:01:16 PST

Created attachment 189229 [details] runs SunSpider, V8, Kraken Still need to test performance. Still need to implement 32-bit. Still need to run LayoutTests.

Filip Pizlo

Comment 19 2013-02-19 21:43:45 PST

(In reply to comment #18) > Created an attachment (id=189229) [details] > runs SunSpider, V8, Kraken > > Still need to test performance. Still need to implement 32-bit. Still need to run LayoutTests. It appears that this is performance-neutral and passes run-javascriptcore-tests. Running LayoutTests now. And then I'll work on 32-bit.

Filip Pizlo

Comment 20 2013-02-20 14:34:21 PST

Created attachment 189382 [details] half-way done with 32-bit, and all LayoutTests pass in 64-bit

Filip Pizlo

Comment 21 2013-02-20 16:30:21 PST

Created attachment 189407 [details] 32-bit is written Still need to test it though.

Filip Pizlo

Comment 22 2013-02-20 20:57:09 PST

Created attachment 189453 [details] the patch

WebKit Review Bot

Comment 23 2013-02-20 21:03:59 PST

Attachment 189453 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/dfg/DFGAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractState.h', u'Source/JavaScriptCore/dfg/DFGAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGAdjacencyList.h', u'Source/JavaScriptCore/dfg/DFGAllocator.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeAllocator.h', u'Source/JavaScriptCore/dfg/DFGNodeFlags.cpp', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.h', u'Source/JavaScriptCore/dfg/DFGValidate.cpp']" exit_code: 1 Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2189: The parameter name "source" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2189: The parameter name "edge" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2190: The parameter name "source" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2190: The parameter name "edge" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2191: The parameter name "source" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2191: The parameter name "edge" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGGraph.h:840: _node is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.h:842: _childIdx is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.h:856: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGGraph.h:862: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 10 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 24 2013-02-20 21:08:36 PST

Created attachment 189455 [details] the patch Rebased, and with some style fixes.

WebKit Review Bot

Comment 25 2013-02-20 21:18:11 PST

Attachment 189455 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/dfg/DFGAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractState.h', u'Source/JavaScriptCore/dfg/DFGAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGAdjacencyList.h', u'Source/JavaScriptCore/dfg/DFGAllocator.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeAllocator.h', u'Source/JavaScriptCore/dfg/DFGNodeFlags.cpp', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.h', u'Source/JavaScriptCore/dfg/DFGValidate.cpp']" exit_code: 1 Source/JavaScriptCore/dfg/DFGGraph.h:840: _node is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.h:842: _childIdx is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.h:856: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGGraph.h:862: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.

Oliver Hunt

Comment 26 2013-02-21 11:24:12 PST

Comment on attachment 189455 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=189455&action=review > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:1298 > + ASSERT(!(value.m_type & ~SpecCell)); // Edge filtering should have already ensured this. What happens if there's a bug and this is wrong? will we handle it safely or should this be a RELEASE_ASSERT? > Source/JavaScriptCore/dfg/DFGNodeAllocator.h:41 > +typedef Allocator<Node, 112> NodeAllocator; > +#else > +typedef Allocator<Node, 80> NodeAllocator; What are these magic numbers?

Filip Pizlo

Comment 27 2013-02-21 12:51:33 PST

(In reply to comment #26) > (From update of attachment 189455 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189455&action=review > > > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:1298 > > + ASSERT(!(value.m_type & ~SpecCell)); // Edge filtering should have already ensured this. > > What happens if there's a bug and this is wrong? will we handle it safely or should this be a RELEASE_ASSERT? We won't handle it safely. If the CFA fails in this kind of fundamental way, then all bets are off. By that same token, I don't expect to see this assertion firing. We'd probably have other obvious symptoms if it did. It could be a RELEASE_ASSERT, but I've put a lot of care into making the CFA fast. This is critical for us. Compiler performance must not regress. This is one of those assertions that I'd rather not bloat release code with. We have ample coverage for this in debug builds. There is a similar assertion via the verifyEdges() mechanism. > > > Source/JavaScriptCore/dfg/DFGNodeAllocator.h:41 > > +typedef Allocator<Node, 112> NodeAllocator; > > +#else > > +typedef Allocator<Node, 80> NodeAllocator; > > What are these magic numbers? These are the current sizes of Node in 32-bit and 64-bit, rounded up to multiple of 16. I thought about making that more implicit - like having Allocator be parameterized by desired alignment - but I like this better because it also acts as an assertion that Node doesn't grow. We generally don't want Node to grow without us finding out about it.

Oliver Hunt

Comment 28 2013-02-21 12:57:11 PST

> We have ample coverage for this in debug builds. There is a similar assertion via the verifyEdges() mechanism. Hmmm. okay. > > > > > > Source/JavaScriptCore/dfg/DFGNodeAllocator.h:41 > > > +typedef Allocator<Node, 112> NodeAllocator; > > > +#else > > > +typedef Allocator<Node, 80> NodeAllocator; > > > > What are these magic numbers? > > These are the current sizes of Node in 32-bit and 64-bit, rounded up to multiple of 16. > > I thought about making that more implicit - like having Allocator be parameterized by desired alignment - but I like this better because it also acts as an assertion that Node doesn't grow. We generally don't want Node to grow without us finding out about it. Can we have a comment to that effect?

Filip Pizlo

Comment 29 2013-02-21 13:00:27 PST

(In reply to comment #28) > > We have ample coverage for this in debug builds. There is a similar assertion via the verifyEdges() mechanism. > > Hmmm. okay. > > > > > > > > > > Source/JavaScriptCore/dfg/DFGNodeAllocator.h:41 > > > > +typedef Allocator<Node, 112> NodeAllocator; > > > > +#else > > > > +typedef Allocator<Node, 80> NodeAllocator; > > > > > > What are these magic numbers? > > > > These are the current sizes of Node in 32-bit and 64-bit, rounded up to multiple of 16. > > > > I thought about making that more implicit - like having Allocator be parameterized by desired alignment - but I like this better because it also acts as an assertion that Node doesn't grow. We generally don't want Node to grow without us finding out about it. > > Can we have a comment to that effect? Yup, added.

Filip Pizlo

Comment 30 2013-02-21 15:01:43 PST

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

Jessie Berlin

Comment 31 2013-02-21 18:52:41 PST

(In reply to comment #30) > Landed in http://trac.webkit.org/changeset/143654 Fixed a typo that broke the 32bit build in http://trac.webkit.org/changeset/143679

Zoltan Arvai

Comment 32 2013-02-22 05:54:33 PST

Some fast/js tests are crashing on 32 bit Qt after this patch. See https://bugs.webkit.org/show_bug.cgi?id=110590

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