[JSC] ArithSub should not propagate "UsesAsOther"
Created attachment 274998 [details] Patch
Conf#1 Conf#2 SunSpider: 3d-cube 4.9307+-0.0957 4.9222+-0.0950 3d-morph 5.1708+-0.0663 ? 5.1783+-0.0467 ? 3d-raytrace 5.5145+-0.0365 ? 5.5304+-0.0690 ? access-binary-trees 2.1499+-0.0374 ? 2.1715+-0.0354 ? might be 1.0101x slower access-fannkuch 5.8879+-0.0826 ? 5.8881+-0.0240 ? access-nbody 2.5250+-0.0280 2.4996+-0.0457 might be 1.0102x faster access-nsieve 3.2002+-0.0695 3.1330+-0.0525 might be 1.0214x faster bitops-3bit-bits-in-byte 1.1306+-0.0221 1.1273+-0.0181 bitops-bits-in-byte 2.7614+-0.0271 ? 2.8000+-0.0639 ? might be 1.0140x slower bitops-bitwise-and 2.0536+-0.0398 2.0227+-0.0314 might be 1.0153x faster bitops-nsieve-bits 3.0996+-0.0253 ? 3.1273+-0.0456 ? controlflow-recursive 2.3840+-0.0648 2.3155+-0.0134 might be 1.0296x faster crypto-aes 3.9912+-0.0714 ? 3.9948+-0.0369 ? crypto-md5 2.5458+-0.1306 2.4669+-0.0393 might be 1.0320x faster crypto-sha1 2.2936+-0.0105 ? 2.3261+-0.0345 ? might be 1.0142x slower date-format-tofte 6.8176+-0.1320 ? 6.9643+-0.1718 ? might be 1.0215x slower date-format-xparb 4.8542+-0.0625 ? 4.9652+-0.1971 ? might be 1.0229x slower math-cordic 2.8032+-0.0174 ? 2.8302+-0.0545 ? math-partial-sums 4.8268+-0.1292 4.8188+-0.0587 math-spectral-norm 1.9849+-0.0231 ? 2.0026+-0.0258 ? regexp-dna 6.2192+-0.0898 ? 6.3581+-0.1502 ? might be 1.0223x slower string-base64 4.3967+-0.0534 ? 4.4772+-0.1214 ? might be 1.0183x slower string-fasta 5.9431+-0.1694 5.8292+-0.0227 might be 1.0195x faster string-tagcloud 8.1152+-0.0659 ? 8.1860+-0.1629 ? string-unpack-code 19.1718+-0.4984 18.8128+-0.3525 might be 1.0191x faster string-validate-input 4.4078+-0.0887 4.3474+-0.0484 might be 1.0139x faster <arithmetic> 4.5838+-0.0270 4.5806+-0.0203 might be 1.0007x faster Conf#1 Conf#2 Octane: encrypt 0.16180+-0.00097 ? 0.16218+-0.00078 ? decrypt 2.79758+-0.00502 ? 2.80383+-0.00867 ? deltablue x2 0.14175+-0.00325 0.14040+-0.00215 earley 0.28110+-0.00106 ? 0.28187+-0.00103 ? boyer 4.93778+-0.04403 ? 4.94451+-0.03648 ? navier-stokes x2 4.92189+-0.00887 4.91822+-0.01628 raytrace x2 0.89016+-0.00361 ? 0.89089+-0.00244 ? richards x2 0.08144+-0.00055 ? 0.08185+-0.00059 ? splay x2 0.35024+-0.00148 0.34965+-0.00165 regexp x2 20.00284+-0.16960 19.88180+-0.20814 pdfjs x2 38.40403+-0.26182 ? 38.74045+-0.33592 ? mandreel x2 42.12221+-0.09619 ? 42.17890+-0.08240 ? gbemu x2 24.01798+-0.18105 ? 24.02125+-0.10917 ? closure 0.54906+-0.00173 0.54882+-0.00211 jquery 7.10231+-0.01710 ? 7.12101+-0.02288 ? box2d x2 9.12832+-0.12747 9.08929+-0.03892 zlib x2 356.50694+-3.60039 354.52699+-5.62696 typescript x2 632.61130+-3.01168 ? 636.36732+-3.26486 ? <geometric> 5.12546+-0.01204 ? 5.12546+-0.00926 ? might be 1.0000x slower Conf#1 Conf#2 Kraken: ai-astar 87.319+-0.789 ? 88.130+-1.996 ? audio-beat-detection 44.910+-0.118 ^ 43.010+-0.426 ^ definitely 1.0442x faster audio-dft 96.392+-0.788 ? 97.927+-1.455 ? might be 1.0159x slower audio-fft 35.294+-0.123 ^ 33.320+-0.365 ^ definitely 1.0593x faster audio-oscillator 47.883+-0.076 ? 48.304+-0.629 ? imaging-darkroom 60.154+-0.090 60.102+-0.061 imaging-desaturate 44.490+-0.213 ? 44.612+-0.668 ? imaging-gaussian-blur 63.341+-1.290 ? 64.255+-1.269 ? might be 1.0144x slower json-parse-financial 36.898+-0.241 ? 37.541+-0.489 ? might be 1.0174x slower json-stringify-tinderbox 25.153+-0.710 ? 25.263+-0.728 ? stanford-crypto-aes 39.040+-0.262 38.996+-0.262 stanford-crypto-ccm 34.603+-0.828 ? 34.609+-1.151 ? stanford-crypto-pbkdf2 97.953+-0.278 ? 98.177+-0.476 ? stanford-crypto-sha256-iterative 38.068+-0.517 37.815+-0.064 <arithmetic> 53.678+-0.195 ? 53.719+-0.248 ? might be 1.0008x slower Conf#1 Conf#2 AsmBench: bigfib.cpp 434.1430+-3.7235 ? 437.7693+-3.5708 ? cray.c 361.0152+-1.4841 ? 361.6810+-1.4657 ? dry.c 447.9220+-24.1240 428.4335+-14.4602 might be 1.0455x faster FloatMM.c 720.2708+-2.0867 719.4121+-5.6026 gcc-loops.cpp 3667.4256+-7.7118 ? 3677.9957+-11.6719 ? n-body.c 809.6624+-1.8275 809.3548+-2.5638 Quicksort.c 390.5018+-0.7997 ? 391.7016+-1.4568 ? stepanov_container.cpp 3270.2863+-9.8415 3253.3661+-12.8740 Towers.c 268.8880+-1.9320 268.1651+-0.9033 <geometric> 718.8807+-3.9822 716.1212+-2.7168 might be 1.0039x faster Conf#1 Conf#2 Geomean of preferred means: <scaled-result> 30.8560+-0.0673 30.8272+-0.0792 might be 1.0009x faster
Comment on attachment 274998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274998&action=review > Source/JavaScriptCore/ChangeLog:21 > + This patch also adds this change and test coverage to ArithAdd. > + ArithAdd was not a problem in practice because it is only > + generated before Fixup if both operands are known to be numerical. > + The change to ArithAdd is there to protect us of the ArithSub-like > + problems if we ever improve our support of arithmetic operators. ArithAdd should only operate on numbers or ints. I think you can make this an assert instead of masking out the UsesAsOther flag. We have already have + operator support in ValueAdd which handles the UsesAsOther case. But is ValueAdd doing the right thing? ValueAdd currently conditionally clears the UsesAsOther flag: if (node->child1()->hasNumberResult() || node->child2()->hasNumberResult()) flags &= ~NodeBytecodeUsesAsOther; Since the ES6 spec also states that the +operator will always ToNumber (in the end) the operands before adding (see https://tc39.github.io/ecma262/2016/#sec-addition-operator-plus-runtime-semantics-evaluation), should we also unconditionally mask out the UsesAsOther flag there too?
Created attachment 275064 [details] Patch
Comment on attachment 275064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275064&action=review r=me. Sorry for my previous confusion with ArithAdd and ValueAdd. > Source/JavaScriptCore/tests/stress/arith-add-on-double-array-with-holes.js:16 > + ['{ toString: function() {Â return ""; } }', NaN, NaN, 1, 1], > + ['{ toString: function() {Â return "WebKit"; } }', NaN, NaN, NaN, NaN], Please remove the non-ASCII character there. > Source/JavaScriptCore/tests/stress/arith-add-on-double-array-with-holes.js:30 > + `// Those holes are not observable by arithmetic operation. Use ' instead of `? > Source/JavaScriptCore/tests/stress/arith-add-on-double-array-with-holes.js:98 > + }` Use ' instead of `? > Source/JavaScriptCore/tests/stress/arith-sub-on-double-array-with-holes.js:16 > + ['{ toString: function() {Â return ""; } }', 1, -1], > + ['{ toString: function() {Â return "WebKit"; } }', NaN, NaN], Please remove the non-ASCII character there. > Source/JavaScriptCore/tests/stress/arith-sub-on-double-array-with-holes.js:28 > + `// Those holes are not observable by arithmetic operation. Use ' instead of `? > Source/JavaScriptCore/tests/stress/arith-sub-on-double-array-with-holes.js:96 > + }` Use ' instead of `? > Source/JavaScriptCore/tests/stress/value-add-on-double-array-with-holes.js:16 > + ['{ toString: function() {Â return ""; } }', '"undefined"', '"undefined"', '"1"', '"1"'], > + ['{ toString: function() {Â return "WebKit"; } }', '"undefinedWebKit"', '"WebKitundefined"', '"1WebKit"', '"WebKit1"'], Please remove the non-ASCII character there. > Source/JavaScriptCore/tests/stress/value-add-on-double-array-with-holes.js:30 > + `// Those holes are not observable by arithmetic operation. Use ' instead of `? > Source/JavaScriptCore/tests/stress/value-add-on-double-array-with-holes.js:98 > + }` Use ' instead of `?
Created attachment 275067 [details] Patch for landing
Comment on attachment 275067 [details] Patch for landing Clearing flags on attachment: 275067 Committed r198770: <http://trac.webkit.org/changeset/198770>
All reviewed patches have been landed. Closing bug.