The lack of support for to_primitive or strcat means that if a code block has expressions like "foo " + bar + " baz" then the DFG cannot optimize the code block at all.
Created attachment 108274 [details] the patch
Attachment 108274 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:170: 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/DFGOperations.h:79: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 108274 [details] the patch Cleared review flag, because I didn't really mean to set it. This is still a work in progress.
Created attachment 108296 [details] the patch When combined with https://bugs.webkit.org/show_bug.cgi?id=68597, it's a 14% speed-up on Kraken. Benchmark report for SunSpider, V8, and Kraken. VMs tested: "TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc "IntDiv" at /Volumes/Data/pizlo/OpenSource/WebKitBuild/Release/jsc "IntDivStrCat" at /Volumes/Data/pizlo/septenary/OpenSource/WebKitBuild/Release/jsc Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. TipOfTree IntDiv IntDivStrCat IntDivStrCat v. TipOfTree SunSpider: 3d-cube 7.7031+-0.2217 ? 7.8395+-0.2910 7.6701+-0.1962 3d-morph 7.5458+-0.1330 7.5094+-0.2132 7.4666+-0.2495 might be 1.0106x faster 3d-raytrace 7.5513+-0.2014 ? 7.8452+-0.3016 7.7827+-0.1491 ? might be 1.0306x slower access-binary-trees 2.0995+-0.0579 2.0684+-0.0596 ? 2.1379+-0.1433 ? might be 1.0183x slower access-fannkuch 11.8476+-0.3710 11.7814+-0.2936 ? 11.8475+-0.3008 access-nbody 3.5674+-0.0885 3.4518+-0.0559 ? 3.5763+-0.0967 ? access-nsieve 2.5801+-0.0738 ? 2.6087+-0.0895 2.5845+-0.0629 ? bitops-3bit-bits-in-byte 1.8394+-0.0448 ? 1.8507+-0.0570 1.8055+-0.0425 might be 1.0188x faster bitops-bits-in-byte 2.8397+-0.0606 ? 2.8577+-0.0704 2.8486+-0.0719 ? bitops-bitwise-and 3.7522+-0.1308 3.6579+-0.1029 3.6303+-0.1049 might be 1.0336x faster bitops-nsieve-bits 5.3488+-0.1198 5.2944+-0.1290 ? 5.4255+-0.1257 ? might be 1.0143x slower controlflow-recursive 2.2455+-0.0651 2.2167+-0.0437 2.2005+-0.0542 might be 1.0204x faster crypto-aes 6.8923+-0.3697 6.3176+-0.2402 6.1573+-0.1484 ^ definitely 1.1194x faster crypto-md5 2.8049+-0.0709 ^ 2.5663+-0.0668 2.4669+-0.0729 ^ definitely 1.1370x faster crypto-sha1 2.3478+-0.0652 ^ 2.1351+-0.0607 ? 2.1476+-0.0806 ^ definitely 1.0932x faster date-format-tofte 10.3161+-0.2810 10.2830+-0.2989 ? 10.4249+-0.2628 ? might be 1.0105x slower date-format-xparb 8.7798+-0.2838 8.7494+-0.2496 ? 8.9220+-0.2599 ? might be 1.0162x slower math-cordic 6.3076+-0.1537 6.1111+-0.1149 6.0745+-0.0764 ^ definitely 1.0384x faster math-partial-sums 7.3659+-0.1267 ? 7.4812+-0.1393 ? 7.6897+-0.2075 ? might be 1.0440x slower math-spectral-norm 2.7030+-0.1058 ! 2.9985+-0.0863 ? 3.0546+-0.0794 ! definitely 1.1300x slower regexp-dna 10.8762+-0.1921 10.8190+-0.1663 ? 10.9030+-0.2224 ? string-base64 5.8533+-0.1371 ? 5.8618+-0.1759 ? 5.9906+-0.2396 ? might be 1.0235x slower string-fasta 7.2075+-0.1744 7.1327+-0.1970 7.1087+-0.1609 might be 1.0139x faster string-tagcloud 11.9179+-0.3311 ? 11.9804+-0.3916 ? 12.0016+-0.3266 ? string-unpack-code 18.7440+-0.3804 ? 18.8508+-0.5263 ? 19.2606+-0.6360 ? might be 1.0276x slower string-validate-input 6.4930+-0.0958 ? 6.6237+-0.2460 6.4775+-0.1372 <arithmetic> 6.4435+-0.0377 6.4189+-0.0338 ? 6.4483+-0.0393 ? <geometric> 5.3328+-0.0319 5.2913+-0.0219 ? 5.3018+-0.0402 <harmonic> 4.3898+-0.0353 4.3388+-0.0330 4.3374+-0.0541 might be 1.0121x faster TipOfTree IntDiv IntDivStrCat IntDivStrCat v. TipOfTree V8: crypto 71.2496+-0.2918 ? 71.3862+-0.3429 71.1184+-0.5196 deltablue 237.3015+-1.4425 ? 237.3798+-1.0827 236.5956+-1.3604 earley-boyer 88.2273+-0.3942 88.1260+-0.3059 87.9492+-0.3159 raytrace 64.1057+-0.6186 ? 65.0615+-0.5036 ^ 62.6563+-0.4224 ^ definitely 1.0231x faster regexp 105.5161+-0.4444 ! 106.6996+-0.3355 ! 108.3552+-0.6639 ! definitely 1.0269x slower richards 198.5336+-0.7411 ? 199.2889+-0.7026 199.0919+-0.6748 ? splay 98.3329+-0.2213 ? 98.5661+-0.5356 98.0713+-0.4491 <arithmetic> 123.3238+-0.2493 ? 123.7869+-0.2621 123.4054+-0.1661 ? <geometric> 110.1665+-0.2139 ! 110.6913+-0.2509 ^ 110.1008+-0.1791 <harmonic> 100.1846+-0.2352 ! 100.7525+-0.2618 ^ 99.8989+-0.2373 TipOfTree IntDiv IntDivStrCat IntDivStrCat v. TipOfTree Kraken: ai-astar 616.6539+-2.9734 611.4591+-4.2299 ? 615.3805+-4.1636 audio-beat-detection 467.0274+-4.0021 ^ 377.9344+-1.6609 ^ 204.5658+-1.0722 ^ definitely 2.2830x faster audio-dft 422.0664+-1.7690 421.1265+-2.1315 419.6968+-2.5885 audio-fft 363.5815+-2.0561 ^ 312.9141+-0.8157 ^ 141.5586+-0.9472 ^ definitely 2.5684x faster audio-oscillator 254.2253+-1.3820 ! 258.3617+-1.3677 ? 258.4752+-1.0439 ! definitely 1.0167x slower imaging-darkroom 422.4535+-1.9687 420.4771+-1.1829 ? 421.7214+-0.7025 imaging-desaturate 208.7662+-0.5285 ? 208.9123+-0.7579 ? 210.1087+-1.2589 ? imaging-gaussian-blur 594.3785+-1.2263 594.0332+-0.8333 ! 598.2852+-3.3919 ? json-parse-financial 48.0277+-0.2236 ! 48.6539+-0.3093 48.6490+-0.2884 ! definitely 1.0129x slower json-stringify-tinderbox 68.8110+-0.2238 ^ 68.0999+-0.3462 ! 70.1338+-0.5934 ! definitely 1.0192x slower stanford-crypto-aes 137.5441+-0.4898 ^ 134.0939+-1.0359 ? 135.0720+-0.7700 ^ definitely 1.0183x faster stanford-crypto-ccm 109.4925+-0.5785 ^ 104.6828+-0.9352 ! 105.8698+-0.2324 ^ definitely 1.0342x faster stanford-crypto-pbkdf2 208.6068+-3.2304 204.5755+-0.9357 ? 205.3682+-1.0130 might be 1.0158x faster stanford-crypto-sha256-iterative 84.5903+-0.2556 ? 84.9048+-0.5000 ? 85.2254+-0.4590 ? <arithmetic> 286.1589+-0.5195 ^ 275.0164+-0.4482 ^ 251.4365+-0.4675 ^ definitely 1.1381x faster <geometric> 217.1089+-0.4842 ^ 210.3265+-0.3279 ^ 191.2333+-0.3325 ^ definitely 1.1353x faster <harmonic> 155.4981+-0.3995 ^ 153.0205+-0.3357 ^ 144.4740+-0.3776 ^ definitely 1.0763x faster TipOfTree IntDiv IntDivStrCat IntDivStrCat v. TipOfTree All benchmarks: <arithmetic> 107.1707+-0.1773 ^ 103.9070+-0.1572 ^ 96.8426+-0.1626 ^ definitely 1.1066x faster <geometric> 25.2527+-0.0971 ^ 24.9250+-0.0673 ^ 24.2354+-0.1112 ^ definitely 1.0420x faster <harmonic> 7.7267+-0.0608 7.6381+-0.0568 7.6281+-0.0927 might be 1.0129x faster
Attachment 108296 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.h:79: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 108297 [details] the patch - fix style
Comment on attachment 108297 [details] the patch - fix style View in context: https://bugs.webkit.org/attachment.cgi?id=108297&action=review > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:166 > + // This is only here so that TrustedImmPtr(0) does not confuse the C++ > + // overload handling rules. I am a little confused by this. Why is it important to be able to compile conversion from the constant 0 and not the constant 1? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1552 > + // FIXME: add string speculation here. Should capitalize the "a" in "Add". > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1611 > + for (int operandIdx = 0; operandIdx < (int)node.numChildren(); ++operandIdx) { Could we use size_t for this type instead of int, or is there a good reason to use int and cast numChildren? Is it just that we don’t want to use a 64-bit index?
(In reply to comment #7) > (From update of attachment 108297 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108297&action=review > > > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:166 > > + // This is only here so that TrustedImmPtr(0) does not confuse the C++ > > + // overload handling rules. > > I am a little confused by this. Why is it important to be able to compile conversion from the constant 0 and not the constant 1? You should never use TrustedImmPtr(1) to assemble an int constant; that's what Imm32() and friends are for. What happened is that if you only have one overload for TrustedImmPtr(), and it takes void*, then you can safely call TrustedImmPtr(0). But when I added a size_t overload for TrustedImmPtr(), it meant that the compiler would reject the conversion from 0 to void*, and loads of code failed to compile. So I added an int overload, but wanted to have this overload assert that it's not being used to store real int32s. > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1552 > > + // FIXME: add string speculation here. > > Should capitalize the "a" in "Add". Got it! > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1611 > > + for (int operandIdx = 0; operandIdx < (int)node.numChildren(); ++operandIdx) { > > Could we use size_t for this type instead of int, or is there a good reason to use int and cast numChildren? Is it just that we don’t want to use a 64-bit index? Yeah, of course, that would be better. I think actually numChildren() is unsigned, so I should use that. (It's unsigned and not size_t because we try to pack things tightly in DFG::Node, and because if we have a DFG with more than 2 billion nodes in it, we're probably dead already.)
Landed in r95758.