Bug 124181 - FTL should have an explicit notion of bytecode liveness
Summary: FTL should have an explicit notion of bytecode liveness
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 118546 124225
Blocks: 124138
  Show dependency treegraph
 
Reported: 2013-11-11 20:12 PST by Filip Pizlo
Modified: 2013-11-17 17:45 PST (History)
15 users (show)

See Also:


Attachments
it begins (28.39 KB, patch)
2013-11-12 13:37 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
slowly getting there (57.28 KB, patch)
2013-11-13 14:27 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
I wrote the code (76.64 KB, patch)
2013-11-14 12:17 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
compiles with FTL disabled (87.62 KB, patch)
2013-11-14 13:21 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it builds (88.74 KB, patch)
2013-11-14 13:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
starting to pass some tests (93.80 KB, patch)
2013-11-14 15:06 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
making some progress (98.19 KB, patch)
2013-11-14 16:37 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (118.40 KB, patch)
2013-11-15 13:10 PST, Filip Pizlo
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
work in progress - new approach (113.94 KB, patch)
2013-11-15 17:58 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (111.67 KB, patch)
2013-11-15 21:38 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (111.37 KB, patch)
2013-11-15 21:40 PST, Filip Pizlo
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
the patch (111.32 KB, patch)
2013-11-15 22:33 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (112.14 KB, patch)
2013-11-16 00:48 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (112.19 KB, patch)
2013-11-16 00:51 PST, Filip Pizlo
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-11-11 20:12:32 PST
I think this is the right way to go, but I'm still thinking it over.
Comment 1 Filip Pizlo 2013-11-12 13:37:58 PST
Created attachment 216717 [details]
it begins

ugh this will be painful
Comment 2 Filip Pizlo 2013-11-13 14:27:26 PST
Created attachment 216862 [details]
slowly getting there
Comment 3 Filip Pizlo 2013-11-14 12:17:25 PST
Created attachment 216966 [details]
I wrote the code

For the most part, anyway.
Comment 4 Filip Pizlo 2013-11-14 13:21:38 PST
Created attachment 216971 [details]
compiles with FTL disabled
Comment 5 Filip Pizlo 2013-11-14 13:35:49 PST
Created attachment 216974 [details]
it builds
Comment 6 Filip Pizlo 2013-11-14 15:06:54 PST
Created attachment 216986 [details]
starting to pass some tests
Comment 7 Filip Pizlo 2013-11-14 16:37:50 PST
Created attachment 216998 [details]
making some progress
Comment 8 Filip Pizlo 2013-11-15 13:10:23 PST
Created attachment 217073 [details]
the patch
Comment 9 WebKit Commit Bot 2013-11-15 13:12:33 PST
Attachment 217073 [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.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysisInlines.h', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/CodeOrigin.cpp', u'Source/JavaScriptCore/bytecode/FullBytecodeLiveness.h', u'Source/JavaScriptCore/dfg/DFGAvailability.cpp', u'Source/JavaScriptCore/dfg/DFGAvailability.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.h', u'Source/JavaScriptCore/dfg/DFGFlushedAt.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGMovHintCountingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGMovHintCountingPhase.h', u'Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGResurrectionForValidationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGResurrectionForValidationPhase.h', u'Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/dfg/DFGVariableAccessDataDump.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExit.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/runtime/DumpContext.cpp', u'Source/JavaScriptCore/runtime/DumpContext.h', u'Source/JavaScriptCore/runtime/Options.h', u'Source/JavaScriptCore/runtime/SymbolTable.h', u'Tools/ChangeLog', u'Tools/Scripts/run-jsc-stress-tests']" exit_code: 1
Source/JavaScriptCore/dfg/DFGAvailability.h:64:  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/DFGGraph.h:787:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:788:  The parameter name "inlineCallFrame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:789:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:59:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:60:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:61:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 EFL EWS Bot 2013-11-15 13:42:18 PST
Comment on attachment 217073 [details]
the patch

Attachment 217073 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/24328008
Comment 11 Mark Hahnenberg 2013-11-15 14:20:47 PST
Comment on attachment 217073 [details]
the patch

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

r=me

> Source/JavaScriptCore/dfg/DFGGraph.cpp:676
> +                if (reg.offset() >= JSStack::CallFrameHeaderSize)
> +                    return true;

Maybe we should ASSERT this instead, since it should be impossible to take this branch.

> Source/JavaScriptCore/dfg/DFGMovHintCountingPhase.h:45
> +// Computes VariableAccessData::m_movHintCount. Variables that are assigned only once can get
> +// treated specially. For example, OSR exit in the FTL can allow the format used for such
> +// nodes BasicBlock::ssa->availabiltiyAtHead/Tail. This is a forward flow type inference
> +// over MovHints. At the time when MovHints are introduced, we were certain that at any
> +// point in the program where a bytecode variable had been live, all of the MovHints that
> +// flowed into that point for that variable had the same FlushFormat. This analysis will tell
> +// you what that format would have been.

Comment seems whacky.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3960
> +        else if (direction == ForwardSpeculation) {

else?

> Source/JavaScriptCore/ftl/FTLOSRExit.cpp:76
> -    
> +

Revert!
Comment 12 EFL EWS Bot 2013-11-15 14:28:15 PST
Comment on attachment 217073 [details]
the patch

Attachment 217073 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/24508008
Comment 13 Filip Pizlo 2013-11-15 16:30:30 PST
Comment on attachment 217073 [details]
the patch

This is a performance regression.  It's because of the bytecode shadow locals.  After talking with mhahnenberg and atrick, I think I'm going to use a different approach.
Comment 14 Filip Pizlo 2013-11-15 17:58:37 PST
Created attachment 217111 [details]
work in progress - new approach
Comment 15 Filip Pizlo 2013-11-15 21:38:21 PST
Created attachment 217113 [details]
the patch
Comment 16 WebKit Commit Bot 2013-11-15 21:39:39 PST
Attachment 217113 [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.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysisInlines.h', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/CodeOrigin.cpp', u'Source/JavaScriptCore/bytecode/FullBytecodeLiveness.h', u'Source/JavaScriptCore/dfg/DFGAvailability.cpp', u'Source/JavaScriptCore/dfg/DFGAvailability.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.h', u'Source/JavaScriptCore/dfg/DFGFlushedAt.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGResurrectionForValidationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGResurrectionForValidationPhase.h', u'Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/ftl/FTLExitValue.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExit.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/llvm/LLVMAPIFunctions.h', u'Source/JavaScriptCore/runtime/DumpContext.cpp', u'Source/JavaScriptCore/runtime/DumpContext.h', u'Source/JavaScriptCore/runtime/Options.h', u'Source/JavaScriptCore/runtime/SymbolTable.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGAvailability.h:84:  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/DFGPlan.cpp:269:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/dfg/DFGGraph.h:787:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:788:  The parameter name "inlineCallFrame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:789:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:59:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:60:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:61:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Filip Pizlo 2013-11-15 21:40:37 PST
Created attachment 217114 [details]
the patch
Comment 18 WebKit Commit Bot 2013-11-15 21:42:23 PST
Attachment 217114 [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.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysisInlines.h', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/CodeOrigin.cpp', u'Source/JavaScriptCore/bytecode/FullBytecodeLiveness.h', u'Source/JavaScriptCore/dfg/DFGAvailability.cpp', u'Source/JavaScriptCore/dfg/DFGAvailability.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.h', u'Source/JavaScriptCore/dfg/DFGFlushedAt.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGResurrectionForValidationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGResurrectionForValidationPhase.h', u'Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/ftl/FTLExitValue.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExit.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/llvm/LLVMAPIFunctions.h', u'Source/JavaScriptCore/runtime/DumpContext.cpp', u'Source/JavaScriptCore/runtime/DumpContext.h', u'Source/JavaScriptCore/runtime/Options.h', u'Source/JavaScriptCore/runtime/SymbolTable.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGAvailability.h:84:  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/DFGPlan.cpp:269:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/dfg/DFGGraph.h:787:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:788:  The parameter name "inlineCallFrame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:789:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:59:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:60:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:61:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 EFL EWS Bot 2013-11-15 21:46:39 PST
Comment on attachment 217114 [details]
the patch

Attachment 217114 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/21948319
Comment 20 EFL EWS Bot 2013-11-15 21:49:57 PST
Comment on attachment 217114 [details]
the patch

Attachment 217114 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/21948321
Comment 21 kov's GTK+ EWS bot 2013-11-15 22:29:07 PST
Comment on attachment 217114 [details]
the patch

Attachment 217114 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/23678257
Comment 22 Filip Pizlo 2013-11-15 22:33:06 PST
Created attachment 217116 [details]
the patch
Comment 23 WebKit Commit Bot 2013-11-15 22:34:23 PST
Attachment 217116 [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.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h', u'Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysisInlines.h', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/CodeOrigin.cpp', u'Source/JavaScriptCore/bytecode/FullBytecodeLiveness.h', u'Source/JavaScriptCore/dfg/DFGAvailability.cpp', u'Source/JavaScriptCore/dfg/DFGAvailability.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.cpp', u'Source/JavaScriptCore/dfg/DFGFlushFormat.h', u'Source/JavaScriptCore/dfg/DFGFlushedAt.cpp', u'Source/JavaScriptCore/dfg/DFGFlushedAt.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/dfg/DFGResurrectionForValidationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGResurrectionForValidationPhase.h', u'Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVariableAccessData.h', u'Source/JavaScriptCore/ftl/FTLExitValue.cpp', u'Source/JavaScriptCore/ftl/FTLInlineCacheSize.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLValueSource.cpp', u'Source/JavaScriptCore/ftl/FTLValueSource.h', u'Source/JavaScriptCore/llvm/LLVMAPIFunctions.h', u'Source/JavaScriptCore/runtime/DumpContext.cpp', u'Source/JavaScriptCore/runtime/DumpContext.h', u'Source/JavaScriptCore/runtime/Options.h', u'Source/JavaScriptCore/runtime/SymbolTable.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGAvailability.h:84:  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/DFGGraph.h:786:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:787:  The parameter name "inlineCallFrame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:788:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:59:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:60:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:61:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Filip Pizlo 2013-11-16 00:48:16 PST
Created attachment 217120 [details]
the patch

fix style
Comment 25 Filip Pizlo 2013-11-16 00:51:55 PST
Created attachment 217121 [details]
the patch
Comment 26 Filip Pizlo 2013-11-16 00:58:25 PST
(In reply to comment #11)
> (From update of attachment 217073 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217073&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGGraph.cpp:676
> > +                if (reg.offset() >= JSStack::CallFrameHeaderSize)
> > +                    return true;
> 
> Maybe we should ASSERT this instead, since it should be impossible to take this branch.

Yup.

> 
> > Source/JavaScriptCore/dfg/DFGMovHintCountingPhase.h:45
> > +// Computes VariableAccessData::m_movHintCount. Variables that are assigned only once can get
> > +// treated specially. For example, OSR exit in the FTL can allow the format used for such
> > +// nodes BasicBlock::ssa->availabiltiyAtHead/Tail. This is a forward flow type inference
> > +// over MovHints. At the time when MovHints are introduced, we were certain that at any
> > +// point in the program where a bytecode variable had been live, all of the MovHints that
> > +// flowed into that point for that variable had the same FlushFormat. This analysis will tell
> > +// you what that format would have been.
> 
> Comment seems whacky.

I killed this phase. ;-)

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3960
> > +        else if (direction == ForwardSpeculation) {
> 
> else?

Yeah.

> 
> > Source/JavaScriptCore/ftl/FTLOSRExit.cpp:76
> > -    
> > +
> 
> Revert!

Yup!
Comment 27 Sam Weinig 2013-11-17 14:22:24 PST
Comment on attachment 217121 [details]
the patch

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

> Source/JavaScriptCore/bytecode/FullBytecodeLiveness.h:35
> +typedef HashMap<unsigned, FastBitVector, WTF::IntHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned> > BytecodeToBitmapMap;

No need for the space between the trailing >>.
Comment 28 Filip Pizlo 2013-11-17 17:45:31 PST
Landed in http://trac.webkit.org/changeset/159394