Bug 85752

Summary: Truncating multiplication on integers should not OSR exit every time
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
barraclough: review-
the patch barraclough: review+

Description Filip Pizlo 2012-05-06 15:34:02 PDT
Say you do a * b where the result is larger than what can fit in an int32, but you use the result in integer contexts only (like [_] | 0).  Currently the DFG will "prove" that the multiplication will produce integers despite overflowing.  But then the multiplication will speculate no overflow, which will typically fail, since in this case a * b does overflow.

Multiplication should be smarter about this.  If you do a * b and we know that it overflows then in the general case our best bet is to perform the multiplication using the FPU.  But if we know something about a or b, for example if we really have something like a * 42, then we know that integer truncation would be equivalent to double truncation - and hence we can perform an integer multiplication and skip the overflow check.
Comment 1 Filip Pizlo 2012-05-06 15:35:57 PDT
Created attachment 140441 [details]
the patch
Comment 2 Gavin Barraclough 2012-05-06 16:19:18 PDT
Comment on attachment 140441 [details]
the patch

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

r-, because I really do think we need a comment here – but nice optimization, looks great otherwise.

> Source/JavaScriptCore/dfg/DFGGraph.h:512
> +        int32_t twoToThe17 = 131072;

Why 2^17?

A number with an absolute value less than 2^17 is representable by 17 bits.  The result of multiplying a value representable by 17 bits by 2^31 (the largest possible absolute value of a signed integer) is representable in a 48 bit mantissa (with additional representation for the sign).  Doubles support 53 bits of mantissa precision.  Put the other way around, for the result of a multiply to be constrained within a 53 bits mantissa, if I know that the absolute value of one operand is no more than 2^31 then the second operand may have a magnitude of +/- 2^22 (less than/greater than – exclusive!).

I think 2^22 is correct here, but either way this really needs a comment. :-)

Also, I'd suggest this would slightly nicer written ''int32_t  twoToThe17 = 1 << 17;".
Comment 3 Filip Pizlo 2012-05-06 16:30:14 PDT
(In reply to comment #2)
> (From update of attachment 140441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140441&action=review
> 
> r-, because I really do think we need a comment here – but nice optimization, looks great otherwise.
> 
> > Source/JavaScriptCore/dfg/DFGGraph.h:512
> > +        int32_t twoToThe17 = 131072;
> 
> Why 2^17?

Yeah, that's sloppy.  It was the largest I tested and I figured it was good enough for the optimization to be effective.  I will make this more rational!
Comment 4 Filip Pizlo 2012-05-06 16:39:22 PDT
Created attachment 140444 [details]
the patch

Incorporated Gavin's feedback.
Comment 5 Filip Pizlo 2012-05-06 20:43:40 PDT
Landed in http://trac.webkit.org/changeset/116264
Comment 6 Filip Pizlo 2012-05-22 10:18:03 PDT
Merged in http://trac.webkit.org/changeset/117993