Bug 124181

Summary:

FTL should have an explicit notion of bytecode liveness

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, commit-queue, eflews.bot, ggaren, gtk-ews, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, nrotem, oliver, rakuco, rego+ews, sam, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

118546, 124225

Bug Blocks:

124138

Attachments:

Description	Flags
it begins	none
slowly getting there	none
I wrote the code	none
compiles with FTL disabled	none
it builds	none
starting to pass some tests	none
making some progress	none
the patch	eflews.bot: commit-queue-
work in progress - new approach	none
the patch	none
the patch	eflews.bot: commit-queue-
the patch	none
the patch	none
the patch	sam: review+

Filip Pizlo

Reported 2013-11-11 20:12:32 PST

I think this is the right way to go, but I'm still thinking it over.

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
Show Obsolete (13) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2013-11-12 13:37:58 PST

Created attachment 216717 [details] it begins ugh this will be painful

Filip Pizlo

Comment 2 2013-11-13 14:27:26 PST

Created attachment 216862 [details] slowly getting there

Filip Pizlo

Comment 3 2013-11-14 12:17:25 PST

Created attachment 216966 [details] I wrote the code For the most part, anyway.

Filip Pizlo

Comment 4 2013-11-14 13:21:38 PST

Created attachment 216971 [details] compiles with FTL disabled

Filip Pizlo

Comment 5 2013-11-14 13:35:49 PST

Created attachment 216974 [details] it builds

Filip Pizlo

Comment 6 2013-11-14 15:06:54 PST

Created attachment 216986 [details] starting to pass some tests

Filip Pizlo

Comment 7 2013-11-14 16:37:50 PST

Created attachment 216998 [details] making some progress

Filip Pizlo

Comment 8 2013-11-15 13:10:23 PST

Created attachment 217073 [details] the patch

WebKit Commit Bot

Comment 9 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.

EFL EWS Bot

Comment 10 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

Mark Hahnenberg

Comment 11 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!

EFL EWS Bot

Comment 12 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

Filip Pizlo

Comment 13 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.

Filip Pizlo

Comment 14 2013-11-15 17:58:37 PST

Created attachment 217111 [details] work in progress - new approach

Filip Pizlo

Comment 15 2013-11-15 21:38:21 PST

Created attachment 217113 [details] the patch

WebKit Commit Bot

Comment 16 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.

Filip Pizlo

Comment 17 2013-11-15 21:40:37 PST

Created attachment 217114 [details] the patch

WebKit Commit Bot

Comment 18 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.

EFL EWS Bot

Comment 19 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

EFL EWS Bot

Comment 20 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

kov's GTK+ EWS bot

Comment 21 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

Filip Pizlo

Comment 22 2013-11-15 22:33:06 PST

Created attachment 217116 [details] the patch

WebKit Commit Bot

Comment 23 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.

Filip Pizlo

Comment 24 2013-11-16 00:48:16 PST

Created attachment 217120 [details] the patch fix style

Filip Pizlo

Comment 25 2013-11-16 00:51:55 PST

Created attachment 217121 [details] the patch

Filip Pizlo

Comment 26 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!

Sam Weinig

Comment 27 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 >>.

Filip Pizlo

Comment 28 2013-11-17 17:45:31 PST

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

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