Bug 162485 – [JSC] Shrink the Math inline caches some more

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

[Benjamin Poulain]

[JSC] Shrink the Math inline caches some more

[Benjamin Poulain]

Created attachment 289673 [details]
Patch

[Saam Barati]

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?

[Saam Barati]

Ah, typing on a phone:
s/around/allowed
s/JOTMathIC/JITMathIC

[Benjamin Poulain]

Created attachment 289738 [details]
Patch

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 289763 [details]
Patch

[Saam Barati]

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.

[Saam Barati]

(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.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Created attachment 289864 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 289864 [details]
Patch for landing

Clearing flags on attachment: 289864

Committed r206392: <http://trac.webkit.org/changeset/206392>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.