Summary: | Truncating multiplication on integers should not OSR exit every time | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Filip Pizlo
2012-05-06 15:34:02 PDT
Created attachment 140441 [details]
the patch
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;". (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! Created attachment 140444 [details]
the patch
Incorporated Gavin's feedback.
Landed in http://trac.webkit.org/changeset/116264 Merged in http://trac.webkit.org/changeset/117993 |