121064 – DFG should support Int52 for local variables

Bug 121064 - DFG should support Int52 for local variables

Summary: DFG should support Int52 for local variables

Status:	RESOLVED FIXED

Alias:	None

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

Importance:	P2 Normal
Assignee:	Filip Pizlo

URL:
Keywords:

Depends on:	121065 121116 121132 121141 121142 121202 121211 121250 121253 121268 121371 121374 121530 121540
Blocks:	121063 121648
	Show dependency tree / graph

Reported:	2013-09-09 18:54 PDT by Filip Pizlo
Modified:	2013-12-08 19:54 PST (History)
CC List:	13 users (show)

See Also:

Attachments
it begins (10.58 KB, patch) 2013-09-11 19:24 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
making some small progress (15.59 KB, patch) 2013-09-12 13:17 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
more (27.10 KB, patch) 2013-09-12 14:44 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
it's getting real (43.50 KB, patch) 2013-09-12 22:16 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
more (48.08 KB, patch) 2013-09-13 18:54 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
starting to get there (60.95 KB, patch) 2013-09-13 21:58 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
filling in the backend (67.60 KB, patch) 2013-09-14 10:33 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
backend almost done (73.31 KB, patch) 2013-09-14 13:13 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
more (78.77 KB, patch) 2013-09-14 18:24 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
almost done! (90.57 KB, patch) 2013-09-15 12:12 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
it ran some easy program (133.97 KB, patch) 2013-09-15 15:16 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
it kind of passes some tests (135.72 KB, patch) 2013-09-15 15:52 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
passes all DFG 64-bit tests (140.88 KB, patch) 2013-09-15 20:24 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
starting on FTL (155.12 KB, patch) 2013-09-15 21:49 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
more FTL (162.23 KB, patch) 2013-09-15 22:13 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
the patch (200.22 KB, patch) 2013-09-16 16:32 PDT, Filip Pizlo	webkit-ews: commit-queue-	Details \| Formatted Diff \| Diff
the patch (202.00 KB, patch) 2013-09-16 18:45 PDT, Filip Pizlo	webkit-ews: commit-queue-	Details \| Formatted Diff \| Diff
the patch (202.78 KB, patch) 2013-09-16 19:05 PDT, Filip Pizlo	webkit-ews: commit-queue-	Details \| Formatted Diff \| Diff
the patch (202.78 KB, patch) 2013-09-16 22:53 PDT, Filip Pizlo	oliver: review+	Details \| Formatted Diff \| Diff
the patch (208.92 KB, patch) 2013-09-17 12:05 PDT, Filip Pizlo	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (19) 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 Filip Pizlo 2013-09-09 18:54:23 PDT

...

Comment 1 Filip Pizlo 2013-09-11 19:24:42 PDT

Created attachment 211385 [details]
it begins

Comment 2 Filip Pizlo 2013-09-12 13:17:24 PDT

Created attachment 211463 [details]
making some small progress

Comment 3 Filip Pizlo 2013-09-12 14:44:01 PDT

Created attachment 211477 [details]
more

Comment 4 Filip Pizlo 2013-09-12 22:16:17 PDT

Created attachment 211507 [details]
it's getting real

Comment 5 Filip Pizlo 2013-09-13 18:54:52 PDT

Created attachment 211614 [details]
more

Comment 6 Filip Pizlo 2013-09-13 21:58:12 PDT

Created attachment 211619 [details]
starting to get there

Comment 7 Filip Pizlo 2013-09-14 10:33:28 PDT

Created attachment 211655 [details]
filling in the backend

Comment 8 Filip Pizlo 2013-09-14 13:13:05 PDT

Created attachment 211666 [details]
backend almost done

Comment 9 Filip Pizlo 2013-09-14 18:24:05 PDT

Created attachment 211679 [details]
more

Comment 10 Filip Pizlo 2013-09-15 12:12:37 PDT

Created attachment 211723 [details]
almost done!

Comment 11 Filip Pizlo 2013-09-15 15:16:07 PDT

Created attachment 211730 [details]
it ran some easy program

Here's some groovy IR, including StrictInt48 addition over Int32 inputs.  Note the lack of an overflow check: adding two int32's to produce an int48 doesn't need it. :-)  Note that this is in debug mode so it probably has some JIT assertions.


Generated DFG JIT code for foo#CZzf6Y:[0x10150aca0->0x10244fe70, DFGFunctionCall], instruction count = 12:
    Optimized with execution counter = 1005.000000/1047.000000, 5
    Code at [0x3b25ef202aa0, 0x3b25ef202bdb):
          0x3b25ef202aa0: pop %rcx
          0x3b25ef202aa1: mov %rcx, 0x10(%r13)
          0x3b25ef202aa5: mov $0x10150aca0, %r11
          0x3b25ef202aaf: mov %r11, 0x8(%r13)
          0x3b25ef202ab3: lea -0x10(%r13), %rdx
          0x3b25ef202ab7: mov $0x101501918, %r11
          0x3b25ef202ac1: cmp %rdx, (%r11)
          0x3b25ef202ac4: ja 0x3b25ef202b18
    Block #0 (bc#0):  (OSR target)
      Predecessors:
      Dominated by: #0
      Dominates: #0
          0x3b25ef202aca: test $0x7, %r13b
          0x3b25ef202ace: jz 0x3b25ef202ad5
          0x3b25ef202ad4: int3 
       1:           < 2:->	SetArgument(, arg1(B~<Int32>/FlushedJSValue), W:SideState, bc#0)  predicting Int32
       2:           < 2:->	SetArgument(, arg2(C~<Int32>/FlushedJSValue), W:SideState, bc#0)  predicting Int32
       3:           < 1:0>	GetLocal(@1, JS|UseAsOther, Int32, arg1(B~<Int32>/FlushedJSValue), R:Variables(8), bc#1)  predicting Int32
          0x3b25ef202ad5: mov 0x40(%r13), %rax
       4:           < 1:-1>	GetLocal(@2, JS|UseAsOther, Int32, arg2(C~<Int32>/FlushedJSValue), R:Variables(9), bc#1)  predicting Int32
          0x3b25ef202ad9: mov 0x48(%r13), %rdx
       5:           <!1:-1>	ValueAdd(Check:MachineInt:@3<Int32>, Check:MachineInt:@4<Int32>, JS|MustGen|MightClobber|UseAsOther|MayOverflow, Int48, bc#1)
          0x3b25ef202add: cmp %r14, %rax
          0x3b25ef202ae0: jb 0x3b25ef202b99
          0x3b25ef202ae6: movsxd %eax, %rax
          0x3b25ef202ae9: cmp %r14, %rdx
          0x3b25ef202aec: jb 0x3b25ef202bba
          0x3b25ef202af2: movsxd %edx, %rdx
          0x3b25ef202af5: add %rdx, %rax
       6:  skipped  < 0:->	ZombieHint(r0(D~<Int48>/FlushedJSValue), W:SideState, bc#1)
       7:           < 1:0>	JSConstant(JS|UseAsOther, Int48, $0 = Double: 4748581863621132288, 3000000000.000000, bc#6)
       8:           <!1:0>	CompareEq(MachineInt:@5<Int48>, MachineInt:@7<Int48>, Boolean|MustGen|MightClobber|UseAsOther|CanExit, Bool, bc#6)
          0x3b25ef202af8: mov $0xb2d05e00, %rdx
          0x3b25ef202b02: cmp %rdx, %rax
          0x3b25ef202b05: setz %al
          0x3b25ef202b08: movzx %al, %eax
          0x3b25ef202b0b: or $0x6, %eax
       9:  skipped  < 0:->	MovHint(@8<Boolean>, r0(E~<Boolean>/FlushedJSValue), W:SideState, bc#6)
      10:           <!0:->	Flush(@2, MustGen, arg2(C~<Int32>/FlushedJSValue), W:SideState, bc#10)  predicting Int32
      11:           <!0:->	Flush(@1, MustGen, arg1(B~<Int32>/FlushedJSValue), W:SideState, bc#10)  predicting Int32
      12:           <!0:->	Return(@8<Boolean>, MustGen, W:SideState, bc#10)
          0x3b25ef202b0e: mov 0x10(%r13), %rdx
          0x3b25ef202b12: mov 0x28(%r13), %r13
          0x3b25ef202b16: push %rdx
          0x3b25ef202b17: ret 
    (End Of Main Path)
          0x3b25ef202b18: mov %rsp, %rdi
          0x3b25ef202b1b: mov %r13, 0x58(%rsp)
          0x3b25ef202b20: mov $0x40000000, 0x34(%r13)
          0x3b25ef202b28: mov $0x10036ea00, %r11
          0x3b25ef202b32: call %r11
          0x3b25ef202b35: jmp 0x3b25ef202aca
          0x3b25ef202b3a: pop %rcx
          0x3b25ef202b3b: mov %rcx, 0x10(%r13)
          0x3b25ef202b3f: mov $0x10150aca0, %r11
          0x3b25ef202b49: mov %r11, 0x8(%r13)
          0x3b25ef202b4d: mov 0x30(%r13), %edx
          0x3b25ef202b51: cmp $0x3, %edx
          0x3b25ef202b54: jae 0x3b25ef202ab3
          0x3b25ef202b5a: mov %rsp, %rdi
          0x3b25ef202b5d: mov %r13, 0x58(%rsp)
          0x3b25ef202b62: mov $0x40000001, 0x34(%r13)
          0x3b25ef202b6a: mov $0x100374060, %r11
          0x3b25ef202b74: call %r11
          0x3b25ef202b77: test %eax, %eax
          0x3b25ef202b79: jz 0x3b25ef202ab3
          0x3b25ef202b7f: mov $0x40000002, 0x34(%r13)
          0x3b25ef202b87: mov $0x3b25ef201940, %r11
          0x3b25ef202b91: call %r11
          0x3b25ef202b94: jmp 0x3b25ef202ab3
          0x3b25ef202b99: test $0x7, %r13b
          0x3b25ef202b9d: jz 0x3b25ef202ba4
          0x3b25ef202ba3: int3 
          0x3b25ef202ba4: mov $0x101813120, %r11
          0x3b25ef202bae: mov $0x0, (%r11)
          0x3b25ef202bb5: jmp 0x3b25ef201c60
          0x3b25ef202bba: test $0x7, %r13b
          0x3b25ef202bbe: jz 0x3b25ef202bc5
          0x3b25ef202bc4: int3 
          0x3b25ef202bc5: mov $0x101813120, %r11
          0x3b25ef202bcf: mov $0x1, (%r11)
          0x3b25ef202bd6: jmp 0x3b25ef201c60
    Structures:

Comment 12 Filip Pizlo 2013-09-15 15:52:22 PDT

Created attachment 211732 [details]
it kind of passes some tests

Comment 13 Filip Pizlo 2013-09-15 20:24:30 PDT

Created attachment 211735 [details]
passes all DFG 64-bit tests

Comment 14 Filip Pizlo 2013-09-15 21:49:32 PDT

Created attachment 211736 [details]
starting on FTL

Comment 15 Filip Pizlo 2013-09-15 22:13:22 PDT

Created attachment 211737 [details]
more FTL

Comment 16 Filip Pizlo 2013-09-16 16:32:01 PDT

Created attachment 211842 [details]
the patch

Comment 17 WebKit Commit Bot 2013-09-16 16:34:10 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h', u'Source/JavaScriptCore/assembler/X86Assembler.h', u'Source/JavaScriptCore/bytecode/DataFormat.h', u'Source/JavaScriptCore/bytecode/OperandsInlines.h', u'Source/JavaScriptCore/bytecode/SpeculatedType.cpp', u'Source/JavaScriptCore/bytecode/SpeculatedType.h', u'Source/JavaScriptCore/bytecode/ValueRecovery.h', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h', u'Source/JavaScriptCore/dfg/DFGAbstractValue.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGArrayMode.cpp', u'Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGClobberize.h', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.h', u'Source/JavaScriptCore/dfg/DFGGenerationInfo.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGJITCode.cpp', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeFlags.h', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSafeToExecute.h', u'Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h', 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/DFGUseKind.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.h', u'Source/JavaScriptCore/dfg/DFGValueSource.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/ftl/FTLCArgumentGetter.cpp', u'Source/JavaScriptCore/ftl/FTLCArgumentGetter.h', u'Source/JavaScriptCore/ftl/FTLCapabilities.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.h', u'Source/JavaScriptCore/ftl/FTLIntrinsicRepository.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLValueFormat.cpp', u'Source/JavaScriptCore/ftl/FTLValueFormat.h', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/interpreter/Register.h', u'Source/JavaScriptCore/runtime/Arguments.cpp', u'Source/JavaScriptCore/runtime/IndexingType.cpp', u'Source/JavaScriptCore/runtime/JSCJSValue.h', u'Source/JavaScriptCore/runtime/JSCJSValueInlines.h']" exit_code: 1
Source/JavaScriptCore/ftl/FTLCArgumentGetter.h:118:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:340:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3372:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4008:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:466:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:858:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 6 in 61 files


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

Comment 18 Early Warning System Bot 2013-09-16 16:42:14 PDT

Comment on attachment 211842 [details]
the patch

Attachment 211842 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1907087

Comment 19 EFL EWS Bot 2013-09-16 17:44:33 PDT

Comment on attachment 211842 [details]
the patch

Attachment 211842 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1859127

Comment 20 Filip Pizlo 2013-09-16 18:45:43 PDT

Created attachment 211853 [details]
the patch

Fix build and actually include WTF changes this time.

Comment 21 WebKit Commit Bot 2013-09-16 18:47:51 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h', u'Source/JavaScriptCore/assembler/X86Assembler.h', u'Source/JavaScriptCore/bytecode/DataFormat.h', u'Source/JavaScriptCore/bytecode/OperandsInlines.h', u'Source/JavaScriptCore/bytecode/SpeculatedType.cpp', u'Source/JavaScriptCore/bytecode/SpeculatedType.h', u'Source/JavaScriptCore/bytecode/ValueRecovery.h', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h', u'Source/JavaScriptCore/dfg/DFGAbstractValue.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGArrayMode.cpp', u'Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGClobberize.h', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.h', u'Source/JavaScriptCore/dfg/DFGGenerationInfo.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGJITCode.cpp', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeFlags.h', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSafeToExecute.h', u'Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h', 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/DFGUseKind.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.h', u'Source/JavaScriptCore/dfg/DFGValueSource.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/ftl/FTLCArgumentGetter.cpp', u'Source/JavaScriptCore/ftl/FTLCArgumentGetter.h', u'Source/JavaScriptCore/ftl/FTLCapabilities.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.h', u'Source/JavaScriptCore/ftl/FTLIntrinsicRepository.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLValueFormat.cpp', u'Source/JavaScriptCore/ftl/FTLValueFormat.h', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/interpreter/Register.h', u'Source/JavaScriptCore/runtime/Arguments.cpp', u'Source/JavaScriptCore/runtime/IndexingType.cpp', u'Source/JavaScriptCore/runtime/JSCJSValue.h', u'Source/JavaScriptCore/runtime/JSCJSValueInlines.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/PrintStream.h']" exit_code: 1
Source/JavaScriptCore/ftl/FTLCArgumentGetter.h:118:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:340:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4009:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:466:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:858:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 6 in 63 files


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

Comment 22 Early Warning System Bot 2013-09-16 18:55:01 PDT

Comment on attachment 211853 [details]
the patch

Attachment 211853 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1844196

Comment 23 Filip Pizlo 2013-09-16 19:05:49 PDT

Created attachment 211854 [details]
the patch

Found and fixed some bugs.

Comment 24 WebKit Commit Bot 2013-09-16 19:07:10 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h', u'Source/JavaScriptCore/assembler/X86Assembler.h', u'Source/JavaScriptCore/bytecode/DataFormat.h', u'Source/JavaScriptCore/bytecode/OperandsInlines.h', u'Source/JavaScriptCore/bytecode/SpeculatedType.cpp', u'Source/JavaScriptCore/bytecode/SpeculatedType.h', u'Source/JavaScriptCore/bytecode/ValueRecovery.h', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h', u'Source/JavaScriptCore/dfg/DFGAbstractValue.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGArrayMode.cpp', u'Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGClobberize.h', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.h', u'Source/JavaScriptCore/dfg/DFGGenerationInfo.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGJITCode.cpp', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeFlags.h', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSafeToExecute.h', u'Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h', 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/DFGUseKind.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.h', u'Source/JavaScriptCore/dfg/DFGValueSource.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/ftl/FTLCArgumentGetter.cpp', u'Source/JavaScriptCore/ftl/FTLCArgumentGetter.h', u'Source/JavaScriptCore/ftl/FTLCapabilities.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.cpp', u'Source/JavaScriptCore/ftl/FTLExitValue.h', u'Source/JavaScriptCore/ftl/FTLIntrinsicRepository.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLValueFormat.cpp', u'Source/JavaScriptCore/ftl/FTLValueFormat.h', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/interpreter/Register.h', u'Source/JavaScriptCore/runtime/Arguments.cpp', u'Source/JavaScriptCore/runtime/IndexingType.cpp', u'Source/JavaScriptCore/runtime/JSCJSValue.h', u'Source/JavaScriptCore/runtime/JSCJSValueInlines.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/PrintStream.h']" exit_code: 1
Source/JavaScriptCore/ftl/FTLCArgumentGetter.h:118:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:340:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4009:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:466:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:858:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 6 in 63 files


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

Comment 25 Early Warning System Bot 2013-09-16 19:16:08 PDT

Comment on attachment 211854 [details]
the patch

Attachment 211854 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1855221

Comment 26 EFL EWS Bot 2013-09-16 20:45:54 PDT

Comment on attachment 211854 [details]
the patch

Attachment 211854 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1893150

Comment 27 EFL EWS Bot 2013-09-16 22:45:40 PDT

Comment on attachment 211854 [details]
the patch

Attachment 211854 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1836342

Comment 28 Filip Pizlo 2013-09-16 22:53:43 PDT

Created attachment 211866 [details]
the patch

Bunch of fixes, to placate GCC and the style bot.

Comment 29 Oliver Hunt 2013-09-17 09:46:40 PDT

Comment on attachment 211866 [details]
the patch

Looks good -- the only thing i'm really uncomfortable with is the Int48s type - why do we need this?  I can't see anywhere that you're not explicitly using the values 1 or 2 and then directly loading indices 0 or 1 - I'm assuming that there's a templatised function somewhere that you pass these to where it's nice to just iterate the list but i can't find it :(

Also Int48s seems like an odd name, i'd prefer Int48Tuple or something.  Then if you wanted you could typedef Int48Pair, etc into existence.

Comment 30 Filip Pizlo 2013-09-17 12:03:58 PDT

(In reply to comment #29)
> (From update of attachment 211866 [details])
> Looks good -- the only thing i'm really uncomfortable with is the Int48s type - why do we need this?  I can't see anywhere that you're not explicitly using the values 1 or 2 and then directly loading indices 0 or 1 - I'm assuming that there's a templatised function somewhere that you pass these to where it's nice to just iterate the list but i can't find it :(
> 
> Also Int48s seems like an odd name, i'd prefer Int48Tuple or something.  Then if you wanted you could typedef Int48Pair, etc into existence.

Agreed, I've fixed this.

Comment 31 Filip Pizlo 2013-09-17 12:04:26 PDT

Why Int48 when you can do Int52?  No reason.  So I changed it.

Comment 32 Filip Pizlo 2013-09-17 12:05:00 PDT

Created attachment 211929 [details]
the patch

Already reviewed by Oliver.

Comment 33 Filip Pizlo 2013-09-17 18:30:41 PDT

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

Comment 34 WebKit Commit Bot 2013-09-17 18:46:11 PDT

Re-opened since this is blocked by bug 121530

Comment 35 Alexey Proskuryakov 2013-09-18 00:26:32 PDT

Rolled out in <https://trac.webkit.org/r156029>:

** The following js test failures have been introduced:
	js/dfg-int-overflow-large-constants-in-a-line

** The following JSC stress test failures have been introduced:
	regress/script-tests/large-int-neg.js.always-trigger-copy-phase

The first failure occurs every time, the second one seems flaky.

Comment 36 Filip Pizlo 2013-09-18 08:24:17 PDT

(In reply to comment #35)
> Rolled out in <https://trac.webkit.org/r156029>:
> 
> ** The following js test failures have been introduced:
>     js/dfg-int-overflow-large-constants-in-a-line

Yeah, that's a bug.  I've got a fix for it.

> 
> ** The following JSC stress test failures have been introduced:
>     regress/script-tests/large-int-neg.js.always-trigger-copy-phase

Not seeing this one.

> 
> The first failure occurs every time, the second one seems flaky.

I think that the second one is caused by the same thing as the first.  I'll test on a couple of machines.  If I don't see the failure in the second one, I'll reland.

Comment 37 Filip Pizlo 2013-09-18 08:58:19 PDT

(In reply to comment #36)
> (In reply to comment #35)
> > Rolled out in <https://trac.webkit.org/r156029>:
> > 
> > ** The following js test failures have been introduced:
> >     js/dfg-int-overflow-large-constants-in-a-line
> 
> Yeah, that's a bug.  I've got a fix for it.
> 
> > 
> > ** The following JSC stress test failures have been introduced:
> >     regress/script-tests/large-int-neg.js.always-trigger-copy-phase
> 
> Not seeing this one.
> 
> > 
> > The first failure occurs every time, the second one seems flaky.
> 
> I think that the second one is caused by the same thing as the first.  I'll test on a couple of machines.  If I don't see the failure in the second one, I'll reland.

I got a different, but related, test to fail.  It was also flaky.  Fix that also.

Comment 38 Filip Pizlo 2013-09-18 10:15:31 PDT

Relanded in http://trac.webkit.org/changeset/156047 with some test fixes:

- OSR entry wasn't handling Int52.  I had assumed that OSR entry seeing Int52 would be impossible; that's sort of true - the baseline JIT won't produce an Int52 and if it produces a double then OSR entry will be disallowed even if that double is representable in Int52.  But we could have the DFG deducing that a local should be an Int52 while the baseline JIT has an Int32 in that variable.  Then we still need the logic to up-convert the Int32 to an Int52.

- Int52 Add and Sub were reusing registers on overflow checks in a way that led to incorrect values.  Disable register reuse for those operations if they do overflow checks.

Comment 39 Ryosuke Niwa 2013-09-19 17:16:39 PDT

This patch caused the bug 121648.

Comment 40 Filip Pizlo 2013-12-08 19:54:12 PST

This was relanded.