Bug 162485 - [JSC] Shrink the Math inline caches some more
Summary: [JSC] Shrink the Math inline caches some more
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-23 02:09 PDT by Benjamin Poulain
Modified: 2016-09-26 14:13 PDT (History)
8 users (show)

See Also:


Attachments
Patch (21.98 KB, patch)
2016-09-23 02:24 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (66.44 KB, patch)
2016-09-23 21:17 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (69.26 KB, patch)
2016-09-24 14:58 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch for landing (66.21 KB, patch)
2016-09-26 13:41 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2016-09-23 02:09:18 PDT
[JSC] Shrink the Math inline caches some more
Comment 1 Benjamin Poulain 2016-09-23 02:24:15 PDT
Created attachment 289673 [details]
Patch
Comment 2 Saam Barati 2016-09-23 09:09:59 PDT
Comment on attachment 289673 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:28
> +        The inline cache average sizes on Sunspider change as follow:

Very nice.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:762
> +    ArithProfile* arithProfile = m_codeBlock->arithProfileForPC(currentInstruction);

I think we should make JITMathIC have an ArithProfile& instead of an ArithProfile* since it's no longer an optional parameter.
Maybe it's also worth having the field be on JOTMathIC instead of each individual generator?
Also, there is code in the DFG/FTL that uses airthProfileForBytecodeOffset and passes in the result to the Generator's
constructor, however, that function is around to return nullptr, so we should assert that it doesn't return nullptr
or we should require the constructor to take ArithProfile& instead of ArithProfile*.

What do you think?
Comment 3 Saam Barati 2016-09-23 09:11:55 PDT
Ah, typing on a phone:
s/around/allowed
s/JOTMathIC/JITMathIC
Comment 4 Benjamin Poulain 2016-09-23 21:17:16 PDT
Created attachment 289738 [details]
Patch
Comment 5 Benjamin Poulain 2016-09-23 21:20:06 PDT
Turns out a null ArithProfile is a legit case. It happens if DFG produces a node from a different bytecode. For example, op_dec can be lowered to ArithSub.

I did some clean up and moved ArithProfile to JITMathIC.
Comment 6 Benjamin Poulain 2016-09-24 14:58:33 PDT
Created attachment 289763 [details]
Patch
Comment 7 Saam Barati 2016-09-24 15:23:31 PDT
Comment on attachment 289763 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:15
> +         of the generators.

"the generators" => "the math IC" since you just changed this.
Comment 8 Saam Barati 2016-09-24 15:25:34 PDT
(In reply to comment #5)
> Turns out a null ArithProfile is a legit case. It happens if DFG produces a
> node from a different bytecode. For example, op_dec can be lowered to
> ArithSub.

Interesting. I didn't think about this situation. I wonder if it's worth it at all to have an ArithProfile for such cases, and just generate an empty IC to begin with? That may be worth it in the case where the decrement has a non-integer argument, but it will be worse in the case of an integer argument. What do you think? If it's worth it, you or me could later write a patch the just gives CodeBlock a Bag<ArithProfile> for such situations. This may be overkill though.
Comment 9 Benjamin Poulain 2016-09-26 13:38:15 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > Turns out a null ArithProfile is a legit case. It happens if DFG produces a
> > node from a different bytecode. For example, op_dec can be lowered to
> > ArithSub.
> 
> Interesting. I didn't think about this situation. I wonder if it's worth it
> at all to have an ArithProfile for such cases, and just generate an empty IC
> to begin with? That may be worth it in the case where the decrement has a
> non-integer argument, but it will be worse in the case of an integer
> argument. What do you think? If it's worth it, you or me could later write a
> patch the just gives CodeBlock a Bag<ArithProfile> for such situations. This
> may be overkill though.

I think you are right, creating an ArithProfile in those cases should yield better codegen. We could populate it from the children's prediction and update when the prediction were optimistic. Alternatively, we could start with the case that got us into UntypedUse and observe the fast cases on the fly.
Comment 10 Benjamin Poulain 2016-09-26 13:41:55 PDT
Created attachment 289864 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2016-09-26 14:13:52 PDT
Comment on attachment 289864 [details]
Patch for landing

Clearing flags on attachment: 289864

Committed r206392: <http://trac.webkit.org/changeset/206392>
Comment 12 WebKit Commit Bot 2016-09-26 14:13:57 PDT
All reviewed patches have been landed.  Closing bug.