Last Comment Bug 762480 - IonMonkey: Inline more unary math functions
: IonMonkey: Inline more unary math functions
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: IonSpeed
  Show dependency treegraph
Reported: 2012-06-07 06:46 PDT by Jan de Mooij [:jandem]
Modified: 2012-06-15 04:34 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (22.23 KB, patch)
2012-06-14 10:28 PDT, Jan de Mooij [:jandem]
dvander: review+
marty.rosenberg: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2012-06-07 06:46:12 PDT
For Kraken darkroom we need Math.log to be fast. On a typical micro-benchmark we're slower than V8.

As a first step we should inline a call to a "double log(MathCache *, double)" helper (and sin, cos, tan). This should be pretty straight-forward and if it does not help much we can try to inline more.
Comment 1 User image Jan de Mooij [:jandem] 2012-06-14 10:28:15 PDT
Created attachment 633179 [details] [diff] [review]

Inlines Math.{log, sin, cos, tan} by calling a stub. Flagging Marty for the ARM changes.

This patch itself is a small win on Kraken darkroom, but combined with some inlining patches this will allow us to hoist a loop-invariant Math.log and that will help a lot more.
Comment 2 User image David Anderson [:dvander] 2012-06-14 18:49:01 PDT
Comment on attachment 633179 [details] [diff] [review]

Review of attachment 633179 [details] [diff] [review]:

::: js/src/ion/CodeGenerator.cpp
@@ +1249,5 @@
> +      default:
> +        JS_NOT_REACHED("Unknown math function");
> +    }
> +
> +    masm.callWithABI(funptr, masm.DOUBLE);

Interesting, this doesn't have to be MacroAssembler::DOUBLE?
Comment 3 User image Jan de Mooij [:jandem] 2012-06-15 04:34:41 PDT

(In reply to David Anderson [:dvander] from comment #2)
> Interesting, this doesn't have to be MacroAssembler::DOUBLE?

Both Clang and GCC 4.2 didn't complain. I changed it to MacroAssembler::DOUBLE though, makes more sense indeed.

Note You need to log in before you can comment on or make changes to this bug.