[webkit-reviews] review requested: [Bug 113862] Negative zero checks cause unnecessary speculation failures on SunSpider math-spectral-norm on ARMv7 : [Attachment 197263] New patch

[bugzilla-daemon at webkit.org]

Roman Zhuykov <zhroma at ispras.ru> has asked  for review:
Bug 113862: Negative zero checks cause unnecessary speculation failures on
SunSpider math-spectral-norm on ARMv7
https://bugs.webkit.org/show_bug.cgi?id=113862

Attachment 197263: New patch
https://bugs.webkit.org/attachment.cgi?id=197263&action=review

------- Additional Comments from Roman Zhuykov <zhroma at ispras.ru>
Comparing SunSpider on x86-64 and ARMv7 I found that ArithDiv nodes cause
unnecessary speculation failures only in math-spectral-norm, but ArithMod nodes
cause them in 8 other tests from SunSpider. Fixing all of them gives
significant SunSpider speedup.
On x86 when ArithMod divides integers "x%y" speculation failure happens only
when "result == 0 && x < 0", but on ARMv7 it checks only "result == 0". The
attached patch fixes the problem. As for ArithDiv, speculation checks for
ArithMod nodes should be created on all platforms only when NodeNeedsNegZero is
set.
In special case on ARMv7s, when second operand is power-of-two-constant, there
were no speculation checks. The same assembly can be used on ARMv7, but we must
insert negative checks on both platforms. Also I found that sometimes
speculationCheck is called with Overflow ExitKind, although is should be
NegativeZero.
And the last -- I see that compileArithNegate calls
nodeCanIgnoreNegativeZero(node->arithNodeFlags()), and it wrongly always
returns true. So, now there are two new node types where arithNodeFlags should
not mask NodeNeedsNegZero -- DoubleAsInt32, ArithNegate. And certainly
ArithMul, ArithDiv, ArithMod.

Now there are three layout tests in the patch:
Negative-zero-divide shows early discussed problem with isNotNegZero
implementation, and it fails on x86 without my patch.
Negative-zero-modulo shows problem with ArithMod, where the second operand is
power-of-two-constant, it should fail on ARMv7s without my patch.
Negative-zero-negate shows problem with ArithNegate, it fails on all platforms
without my patch.

Here are performance results for ARMv7.

TEST		       COMPARISON	     FROM		  TO	       
 DETAILS

=============================================================================

** TOTAL **:	       1.050x as fast	 1473.7ms +/- 0.7%   1403.9ms +/- 0.6% 
   significant

=============================================================================

  3d:		       1.081x as fast	  267.3ms +/- 1.8%    247.3ms +/- 2.0% 
   significant
    cube:	       -		   96.4ms +/- 2.3%     96.3ms +/- 2.5% 

    morph:	       -		   44.6ms +/- 0.8%     44.6ms +/- 1.1% 

    raytrace:	       1.187x as fast	  126.3ms +/- 2.4%    106.4ms +/- 2.2% 
   significant

  access:	       -		  149.3ms +/- 1.6%    149.0ms +/- 1.3% 

    binary-trees:      -		   27.8ms +/- 2.4%     27.7ms +/- 2.7% 

    fannkuch:	       -		   50.3ms +/- 1.0%     50.3ms +/- 0.7% 

    nbody:	       -		   47.2ms +/- 2.5%     47.0ms +/- 2.2% 

    nsieve:	       -		   24.0ms +/- 1.4%     24.0ms +/- 1.4% 


  bitops:	       -		   91.0ms +/- 0.9%     91.0ms +/- 0.9% 

    3bit-bits-in-byte: -		   16.7ms +/- 2.1%     16.7ms +/- 2.1% 

    bits-in-byte:      -		   23.5ms +/- 1.6%     23.5ms +/- 1.6% 

    bitwise-and:       -		   23.0ms +/- 1.5%     22.7ms +/- 1.5% 

    nsieve-bits:       ??		   27.8ms +/- 1.1%     28.1ms +/- 1.4% 
   not conclusive: might be *1.011x as slow*

  controlflow:	       -		   25.0ms +/- 2.7%     24.7ms +/- 2.0% 

    recursive:	       -		   25.0ms +/- 2.7%     24.7ms +/- 2.0% 


  crypto:	       1.166x as fast	  175.2ms +/- 1.6%    150.3ms +/- 1.5% 
   significant
    aes:	       1.179x as fast	   80.5ms +/- 1.4%     68.3ms +/- 1.3% 
   significant
    md5:	       1.166x as fast	   56.8ms +/- 1.9%     48.7ms +/- 2.5% 
   significant
    sha1:	       1.138x as fast	   37.9ms +/- 1.9%     33.3ms +/- 1.0% 
   significant

  date: 	       1.030x as fast	  170.8ms +/- 1.4%    165.8ms +/- 1.2% 
   significant
    format-tofte:      -		   87.5ms +/- 1.6%     87.2ms +/- 1.3% 

    format-xparb:      1.060x as fast	   83.3ms +/- 1.6%     78.6ms +/- 1.4% 
   significant

  math: 	       1.080x as fast	  121.5ms +/- 0.9%    112.5ms +/- 1.3% 
   significant
    cordic:	       -		   33.6ms +/- 1.5%     33.1ms +/- 1.6% 

    partial-sums:      -		   51.9ms +/- 0.8%     51.8ms +/- 1.1% 

    spectral-norm:     1.30x as fast	   36.0ms +/- 1.3%     27.6ms +/- 2.2% 
   significant

  regexp:	       ??		   69.7ms +/- 0.5%     69.9ms +/- 0.6% 
   not conclusive: might be *1.003x as slow*
    dna:	       ??		   69.7ms +/- 0.5%     69.9ms +/- 0.6% 
   not conclusive: might be *1.003x as slow*

  string:	       1.027x as fast	  403.9ms +/- 0.5%    393.4ms +/- 0.8% 
   significant
    base64:	       -		   38.8ms +/- 1.2%     38.8ms +/- 1.7% 

    fasta:	       1.010x as fast	   68.0ms +/- 0.7%     67.3ms +/- 0.7% 
   significant
    tagcloud:	       -		   94.7ms +/- 1.4%     94.0ms +/- 1.2% 

    unpack-code:       1.019x as fast	  148.0ms +/- 0.7%    145.2ms +/- 0.6% 
   significant
    validate-input:    1.131x as fast	   54.4ms +/- 0.7%     48.1ms +/- 0.8% 
   significant