944398 - IonMonkey: inline Math.fround even if it hasn't been executed

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Skinning Float32

When running the awfy asm.js benchmarks compiled with Float32 support (kudos to alon!), there is a big slowdown on skinning.js if we run it with --no-asmjs (i.e. only in Ion).

Replacing Math.fround by the naive polyfill (Math.fround = function(x) { return x }) makes the enclosed test case 40% faster.

Investigation shows that this happens because the observed return type set of Math.fround is empty at compile time, and thus Math.fround isn't inlined because it expects an Double return type, and then:
1) Math.fround is called in the interpreter directly a big amount of times,
2) Float32 optimizations aren't activated before a recompilation.

When looking at the code of IonBuilder::jsop_call, if the observed type set of a function is empty, we try to give a look at its uses to determine its return type (e.g. f()|0 will have an integer return type).

My proposal: Some functions that we inline (Math.fround for this example, but also Math.sqrt, etc.) only have one possible return typeset (must be double in both cases). For these functions, could we just say that we can inline them if the observed return type is the one expected (e.g. double, here) OR the observed typeset is empty (and in this case, add the return type to the observed typeset)?

Making this for Math.fround almost closes up the gap between the naive polyfill and the actual fround implementation.

I am not familiar with type inference though, so maybe that's crazy. Brian, what do you think?

Comment 1

[Boris Zbarsky [:bzbarsky]]

We could do this for DOM methods too, via jitinfo.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Needinfo'ing Brian as per description.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #0)
> When looking at the code of IonBuilder::jsop_call, if the observed type set
> of a function is empty, we try to give a look at its uses to determine its
> return type (e.g. f()|0 will have an integer return type).
> 
> My proposal: Some functions that we inline (Math.fround for this example,
> but also Math.sqrt, etc.) only have one possible return typeset (must be
> double in both cases). For these functions, could we just say that we can
> inline them if the observed return type is the one expected (e.g. double,
> here) OR the observed typeset is empty (and in this case, add the return
> type to the observed typeset)?

I think it'd be better if, when trying to inline these natives, IonBuilder just updated empty observed type sets with the types it thinks that call will produce.  IonBuilder is always free to update observed type sets for opcodes it is compiling.  In this case IonBuilder::inlineMathFRound could call getInlineReturnTypeSet and add the double type to it if it is empty (or even if it isn't empty).  Generally, IonBuilder should make sure that it updates observed type sets to reflect the types it is actually assuming during compilation.  This makes the most difference with off thread compilation, as if IonBuilder didn't update the observed type set and the fround() call executes in baseline between when Ion compilation starts and when it finishes, the new type added by the call in baseline would invalidate the compilation.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch for Math.fround

Thanks Brian! If that patch looks good to you, I'll generalize it to other functions that only return doubles (math trigo functions, for instances) and benchmark the results.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8341997 [details] [diff] [review]
Patch for Math.fround

Actually, I think that'd be nice to have this optimization for Math.fround quickly in the tree. I'll make the patch for the other functions later and will let this bug open in the mean time.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4d9d11d0be

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2a4d9d11d0be