Bug 175396

Summary: Support compiling catch in the FTL
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 174590, 176060    
Bug Blocks: 175397, 176336    
Attachments:
Description Flags
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
buildbot: commit-queue-
WIP
none
WIP
none
WIP
none
patch
fpizlo: review+
patch for landing
none
patch for landing none

Description Saam Barati 2017-08-09 14:28:16 PDT 
...
Comment 1 Saam Barati 2017-08-24 12:41:31 PDT 
Created attachment 319011 [details]
WIP

it begins
Comment 2 Saam Barati 2017-08-24 16:57:04 PDT 
Created attachment 319039 [details]
WIP
Comment 3 Saam Barati 2017-08-24 17:17:53 PDT 
Created attachment 319044 [details]
WIP
Comment 4 Saam Barati 2017-08-24 22:16:05 PDT 
Created attachment 319063 [details]
WIP
Comment 5 Saam Barati 2017-08-25 16:22:08 PDT 
Created attachment 319111 [details]
WIP

Not using EntrySwitch for catch yet, but much of it is wired through the DFG at this point.
Comment 6 Saam Barati 2017-08-25 18:10:04 PDT 
Created attachment 319124 [details]
WIP
Comment 7 Saam Barati 2017-08-25 18:33:34 PDT 
Created attachment 319127 [details]
WIP

A bunch of the try/catch microbenchmarks run. I'm running the full test suite now.
Comment 8 Build Bot 2017-08-25 19:01:31 PDT 
Attachment 319127 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:265:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:287:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:292:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:293:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:300:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:309:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:66:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:67:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLState.cpp:83:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Build Bot 2017-08-26 05:40:51 PDT 
Comment on attachment 319127 [details]
WIP

Attachment 319127 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4385073

New failing tests:
microbenchmarks/delta-blue-try-catch.js.ftl-eager-no-cjit
stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager-no-cjit
stress/typedarray-bad-getter.js.ftl-eager-no-cjit-b3o1
stress/tail-call-recognize.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/unicode-escape-sequences.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/object-literal-shorthand-construction.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager
stress/activation-sink-default-value-tdz-error.js.ftl-eager-no-cjit
stress/typedarray-access-monomorphic-neutered.js.ftl-eager
stress/typedarray-access-monomorphic-neutered.js.ftl-eager-no-cjit-b3o1
stress/const-tdz.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/exception-expression-offset.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-error-messages.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/array-proto-func-property-getter-except.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/strict/23.reservedWords.js.default
stress/activation-sink-default-value-tdz-error.js.ftl-eager-no-cjit-b3o1
stress/typedarray-access-monomorphic-neutered.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-eager-no-cjit
stress/typedarray-functions-with-neutered.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/es6-function-properties.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager
stress/typedarray-functions-with-neutered.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/typedarray-prototype.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/object-literal-duplicate-properties.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/array-proto-func-length-getter-except.js.layout-ftl-eager-no-cjit
stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/regress-150513.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/resolve-arguments-from-scope.js.layout-ftl-eager-no-cjit
stress/const-tdz.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/regress-150434.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/strict/23.reservedWords_sm.js.default
jsc-layout-tests.yaml/js/script-tests/regress-150434.js.layout
jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-all-the-places-broken.js.layout-ftl-eager-no-cjit
stress/typedarray-bad-getter.js.ftl-eager-no-cjit
stress/typedarray-functions-with-neutered.js.default
jsc-layout-tests.yaml/js/script-tests/regress-150434.js.layout-ftl-no-cjit
stress/typedarray-functions-with-neutered.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/intl-collator.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
stress/activation-sink-default-value-tdz-error.js.ftl-eager
stress/typedarray-functions-with-neutered.js.ftl-no-cjit-validate-sampling-profiler
stress/typedarray-functions-with-neutered.js.ftl-no-cjit-no-inline-validate
stress/typedarray-access-neutered.js.ftl-eager
stress/class-syntax-super-in-eval.js.ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/basic-strict-mode.js.layout-ftl-eager-no-cjit
stress/typedarray-functions-with-neutered.js.ftl-no-cjit-b3o1
stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-eager
stress/eval-func-decl-in-global-of-eval.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/dfg-compare-final-object-to-final-object-or-other-when-both-proven-final-object.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager
stress/tail-call-recognize.js.ftl-eager-no-cjit
stress/typedarray-access-neutered.js.ftl-eager-no-cjit-b3o1
stress/typedarray-bad-getter.js.ftl-eager
stress/class-syntax-super-in-eval.js.ftl-eager-no-cjit-b3o1
stress/const-semantics.js.ftl-eager
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/dfg-compare-final-object-to-final-object-or-other-when-proven-final-object.js.layout-ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/Miscellaneous/HasOnlyWritableDataPropertiesCache.js.default
stress/const-semantics.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/date-toLocaleString.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-syntax-errors.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager-no-cjit
microbenchmarks/delta-blue-try-catch.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/statement-list-item-syntax-errors.js.layout-ftl-eager-no-cjit
stress/tail-call-recognize.js.ftl-eager
stress/const-semantics.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/setPrototypeOf.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-ftl-eager-no-cjit
stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-eager-no-cjit
stress/eval-func-decl-in-global-of-eval.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/symbol-abstract-relational-comparison.js.layout-ftl-eager-no-cjit
stress/typedarray-functions-with-neutered.js.ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/date-proto-generic-invocation.js.layout-ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/Lib/construct.js.default
stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/reserved-words.js.layout-ftl-eager-no-cjit
stress/typedarray-access-neutered.js.ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-ftl-eager-no-cjit
Comment 10 Build Bot 2017-08-26 07:37:41 PDT 
Comment on attachment 319127 [details]
WIP

Attachment 319127 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4385462

New failing tests:
microbenchmarks/delta-blue-try-catch.js.ftl-eager-no-cjit
stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager-no-cjit
stress/typedarray-bad-getter.js.ftl-eager-no-cjit-b3o1
stress/tail-call-recognize.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/unicode-escape-sequences.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/object-literal-shorthand-construction.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager
stress/activation-sink-default-value-tdz-error.js.ftl-eager-no-cjit
stress/typedarray-access-monomorphic-neutered.js.ftl-eager
stress/typedarray-access-monomorphic-neutered.js.ftl-eager-no-cjit-b3o1
stress/const-tdz.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/exception-expression-offset.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-error-messages.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/array-proto-func-property-getter-except.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/strict/23.reservedWords.js.default
stress/activation-sink-default-value-tdz-error.js.ftl-eager-no-cjit-b3o1
stress/typedarray-access-monomorphic-neutered.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-eager-no-cjit
stress/typedarray-functions-with-neutered.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/es6-function-properties.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager
stress/typedarray-functions-with-neutered.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/typedarray-prototype.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/object-literal-duplicate-properties.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/array-proto-func-length-getter-except.js.layout-ftl-eager-no-cjit
stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/regress-150513.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/resolve-arguments-from-scope.js.layout-ftl-eager-no-cjit
stress/const-tdz.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/regress-150434.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/strict/23.reservedWords_sm.js.default
jsc-layout-tests.yaml/js/script-tests/regress-150434.js.layout
jsc-layout-tests.yaml/js/script-tests/dfg-inline-arguments-use-from-all-the-places-broken.js.layout-ftl-eager-no-cjit
stress/typedarray-bad-getter.js.ftl-eager-no-cjit
stress/typedarray-functions-with-neutered.js.default
jsc-layout-tests.yaml/js/script-tests/regress-150434.js.layout-ftl-no-cjit
stress/typedarray-functions-with-neutered.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/intl-collator.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
stress/activation-sink-default-value-tdz-error.js.ftl-eager
stress/typedarray-functions-with-neutered.js.ftl-no-cjit-validate-sampling-profiler
stress/typedarray-functions-with-neutered.js.ftl-no-cjit-no-inline-validate
stress/typedarray-access-neutered.js.ftl-eager
stress/class-syntax-super-in-eval.js.ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/basic-strict-mode.js.layout-ftl-eager-no-cjit
stress/typedarray-functions-with-neutered.js.ftl-no-cjit-b3o1
stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-eager
stress/eval-func-decl-in-global-of-eval.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-block-with-var-sinthesize.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/dfg-compare-final-object-to-final-object-or-other-when-both-proven-final-object.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager
stress/tail-call-recognize.js.ftl-eager-no-cjit
stress/typedarray-access-neutered.js.ftl-eager-no-cjit-b3o1
stress/typedarray-bad-getter.js.ftl-eager
stress/class-syntax-super-in-eval.js.ftl-eager-no-cjit-b3o1
stress/const-semantics.js.ftl-eager
stress/eval-func-decl-in-eval-within-with-scope.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/dfg-compare-final-object-to-final-object-or-other-when-proven-final-object.js.layout-ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/Miscellaneous/HasOnlyWritableDataPropertiesCache.js.default
stress/const-semantics.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/date-toLocaleString.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-syntax-errors.js.layout-ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager-no-cjit
microbenchmarks/delta-blue-try-catch.js.ftl-eager-no-cjit-b3o1
stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/statement-list-item-syntax-errors.js.layout-ftl-eager-no-cjit
stress/tail-call-recognize.js.ftl-eager
stress/const-semantics.js.ftl-eager-no-cjit
stress/const-tdz.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/setPrototypeOf.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-ftl-eager-no-cjit
stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-eager-no-cjit
stress/eval-func-decl-in-global-of-eval.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/symbol-abstract-relational-comparison.js.layout-ftl-eager-no-cjit
stress/typedarray-functions-with-neutered.js.ftl-eager-no-cjit
stress/eval-func-decl-in-eval-within-block-with-let.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/date-proto-generic-invocation.js.layout-ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/Lib/construct.js.default
stress/eval-func-decl-within-eval-with-reassign-to-var.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/reserved-words.js.layout-ftl-eager-no-cjit
stress/typedarray-access-neutered.js.ftl-eager-no-cjit
stress/eval-func-decl-block-with-var-and-remove.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-ftl-eager-no-cjit
Comment 11 Saam Barati 2017-08-28 14:53:54 PDT 
Created attachment 319211 [details]
WIP
Comment 12 Build Bot 2017-08-28 14:56:43 PDT 
Attachment 319211 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:265:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:287:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:292:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:293:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:300:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:309:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLState.cpp:83:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOSREntry.cpp:174:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:446:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:447:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:448:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 11 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Saam Barati 2017-08-28 17:38:29 PDT 
Created attachment 319222 [details]
WIP
Comment 14 Build Bot 2017-08-28 17:42:41 PDT 
Attachment 319222 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:167:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:289:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:311:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:316:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:317:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:324:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:333:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOSREntry.cpp:343:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGCommonData.cpp:200:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:446:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:447:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:448:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 12 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Saam Barati 2017-08-28 19:19:31 PDT 
Created attachment 319231 [details]
WIP
Comment 16 Saam Barati 2017-08-29 17:12:35 PDT 
Created attachment 319312 [details]
patch
Comment 17 Build Bot 2017-08-29 17:16:03 PDT 
Attachment 319312 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:168:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGCommonData.cpp:200:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:445:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:446:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:447:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Filip Pizlo 2017-08-31 16:28:06 PDT 
Comment on attachment 319312 [details]
patch

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

r=me with comments

> JSTests/microbenchmarks/fake-iterators-that-throw-when-finished.js:76
> -    if (true)
> +    if (false)

lol

> Source/JavaScriptCore/b3/air/AirGenerate.cpp:226
> +                jit.emitFunctionPrologue();
> +                if (code.frameSize()) {
> +                    AllowMacroScratchRegisterUsageIf allowScratch(jit, isARM64());
> +                    jit.addPtr(CCallHelpers::TrustedImm32(-code.frameSize()), MacroAssembler::stackPointerRegister);
> +                }
> +                
> +                jit.emitSave(code.calleeSaveRegisterAtOffsetList());

Why not have Code set up default prologue generators that do this?

> Source/JavaScriptCore/dfg/DFGGraph.h:1014
> +        // In SSA, this is the argument speculation that we've locked in for an entrypoint block.
> +        //
> +        // We must speculate on the argument types at each entrypoint even if operations involving
> +        // arguments get killed. For example:
> +        //
> +        //     function foo(x) {
> +        //        var tmp = x + 1;
> +        //     }
> +        //
> +        // Assume that x is always int during profiling. The ArithAdd for "x + 1" will be dead and will
> +        // have a proven check for the edge to "x". So, we will not insert a Check node and we will
> +        // kill the GetStack for "x". But, we must do the int check in the progolue, because that's the
> +        // thing we used to allow DCE of ArithAdd. Otherwise the add could be impure:
> +        //
> +        //     var o = {
> +        //         valueOf: function() { do side effects; }
> +        //     };
> +        //     foo(o);
> +        //
> +        // If we DCE the ArithAdd and we remove the int check on x, then this won't do the side
> +        // effects.
> +        //
> +        // By convention, entrypoint index 0 is used for the CodeBlock's op_enter entrypoint.
> +        // So argumentFormats[0] are the argument formats for the normal call entrypoint.
> +        Vector<Vector<FlushFormat>> argumentFormats;
> +
> +        // This maps an entrypoint index to a particular op_catch bytecode offset. By convention,
> +        // it'll never have zero as a key because we use zero to mean the op_enter entrypoint.
> +        HashMap<unsigned, unsigned> entrypointIndexToCatchBytecodeOffset;
> +
> +        unsigned numberOfEntrypoints { UINT_MAX };
> +    };
> +    std::unique_ptr<SSAData> m_ssaData;

There's a lot of stuff in Graph that is SSA-only and it's not in a structure like this.  Any reason why this can't just be directly on Graph?
Comment 19 Saam Barati 2017-09-04 14:43:14 PDT 
Comment on attachment 319312 [details]
patch

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

>> Source/JavaScriptCore/b3/air/AirGenerate.cpp:226
>> +                jit.emitSave(code.calleeSaveRegisterAtOffsetList());
> 
> Why not have Code set up default prologue generators that do this?

I don't follow exactly what you're proposing here. Do you want Code to add a default generator for each index in its own HashMap? Or do you want it to have functions that know how to adjust SP and spill callee saves?

>> Source/JavaScriptCore/dfg/DFGGraph.h:1014
>> +    std::unique_ptr<SSAData> m_ssaData;
> 
> There's a lot of stuff in Graph that is SSA-only and it's not in a structure like this.  Any reason why this can't just be directly on Graph?

Filip and I spoke offline. I may do this in a follow up patch for all the SSA data.
Comment 20 Saam Barati 2017-09-04 15:30:40 PDT 
Created attachment 319863 [details]
patch for landing
Comment 21 Build Bot 2017-09-04 15:33:16 PDT 
Attachment 319863 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:169:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGCommonData.cpp:200:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:444:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:445:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Saam Barati 2017-09-04 16:06:56 PDT 
Created attachment 319864 [details]
patch for landing
Comment 23 Build Bot 2017-09-04 16:09:35 PDT 
Attachment 319864 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:169:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGCommonData.cpp:200:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:444:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:445:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Filip Pizlo 2017-09-04 20:19:44 PDT 
(In reply to Saam Barati from comment #19)
> Comment on attachment 319312 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319312&action=review
> 
> >> Source/JavaScriptCore/b3/air/AirGenerate.cpp:226
> >> +                jit.emitSave(code.calleeSaveRegisterAtOffsetList());
> > 
> > Why not have Code set up default prologue generators that do this?
> 
> I don't follow exactly what you're proposing here. Do you want Code to add a
> default generator for each index in its own HashMap? Or do you want it to
> have functions that know how to adjust SP and spill callee saves?

I want a Code to add a default generator for each index. Then it doesn’t even need to be a hashmap. It would just be a vector. Maybe we should then combine it with the other entry point indexed vectors (like m_entrypointLabels) and have it be a vector of structs. 

> 
> >> Source/JavaScriptCore/dfg/DFGGraph.h:1014
> >> +    std::unique_ptr<SSAData> m_ssaData;
> > 
> > There's a lot of stuff in Graph that is SSA-only and it's not in a structure like this.  Any reason why this can't just be directly on Graph?
> 
> Filip and I spoke offline. I may do this in a follow up patch for all the
> SSA data.
Comment 25 WebKit Commit Bot 2017-09-04 20:21:37 PDT 
Comment on attachment 319864 [details]
patch for landing

Clearing flags on attachment: 319864

Committed r221602: <http://trac.webkit.org/changeset/221602>
Comment 26 WebKit Commit Bot 2017-09-04 20:21:39 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 27 Saam Barati 2017-09-04 20:27:07 PDT 
(In reply to Filip Pizlo from comment #24)
> (In reply to Saam Barati from comment #19)
> > Comment on attachment 319312 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=319312&action=review
> > 
> > >> Source/JavaScriptCore/b3/air/AirGenerate.cpp:226
> > >> +                jit.emitSave(code.calleeSaveRegisterAtOffsetList());
> > > 
> > > Why not have Code set up default prologue generators that do this?
> > 
> > I don't follow exactly what you're proposing here. Do you want Code to add a
> > default generator for each index in its own HashMap? Or do you want it to
> > have functions that know how to adjust SP and spill callee saves?
> 
> I want a Code to add a default generator for each index. Then it doesn’t
> even need to be a hashmap. It would just be a vector. Maybe we should then
> combine it with the other entry point indexed vectors (like
> m_entrypointLabels) and have it be a vector of structs. 

Ok. I'll look into this in a followup. What makes it a bit tricky is that Air doesn't know about all the entrypoints in the vector until after LowerEntrySwitch. However, I want this API to be callable even before we emit B3 code.

> 
> > 
> > >> Source/JavaScriptCore/dfg/DFGGraph.h:1014
> > >> +    std::unique_ptr<SSAData> m_ssaData;
> > > 
> > > There's a lot of stuff in Graph that is SSA-only and it's not in a structure like this.  Any reason why this can't just be directly on Graph?
> > 
> > Filip and I spoke offline. I may do this in a follow up patch for all the
> > SSA data.
Comment 28 Radar WebKit Bug Importer 2017-09-27 12:50:35 PDT 
<rdar://problem/34694123>