Bug 155932 – [JSC] ArithSub should not propagate "UsesAsOther"

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[JSC] ArithSub should not propagate "UsesAsOther"

[Benjamin Poulain]

Created attachment 274998 [details]
Patch

[Benjamin Poulain]

                                                  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

[Mark Lam]

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?

[Benjamin Poulain]

Created attachment 275064 [details]
Patch

[Mark Lam]

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 `?

[Benjamin Poulain]

Created attachment 275067 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 275067 [details]
Patch for landing

Clearing flags on attachment: 275067

Committed r198770: <http://trac.webkit.org/changeset/198770>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.