918163 - IonMonkey: specialize MMathFunction for Float32

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

So that Math.fround(cos(Math.fround(1)) uses cosf and not cos.

Comment 1

[Luke Wagner [:luke]]

OOC, will the float analysis recognize literals that are exactly representable as floats like 1 or 1.5 as float producers?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #1)
> OOC, will the float analysis recognize literals that are exactly
> representable as floats like 1 or 1.5 as float producers?

Yes.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Some maths functions

Here are some specializations for some math functions. It looks like the other ones are not standard, so some stub implementations should be made in jsmath and the build system should detect which one are to implement.

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 807555 [details] [diff] [review]
Some maths functions

Review of attachment 807555 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: js/src/jit/CodeGenerator.cpp
@@ +3815,5 @@
> +    void *funptr = NULL;
> +    switch (ins->mir()->function()) {
> +      case MMathFunction::Log:
> +        funptr = JS_FUNC_TO_DATA_PTR(void *, logf);
> +        break;

This would look better condensed to a single line:
|case MMathFunction::Log: funptr = ...; break;|

@@ +3849,5 @@
> +      case MMathFunction::ASinH:
> +      case MMathFunction::ATanH:
> +      case MMathFunction::Sign:
> +      case MMathFunction::Trunc:
> +      case MMathFunction::Cbrt:

It isn't necessary to list the unhandled cases explicitly -- "default" is fine.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fixed nits

Thanks for the quick review!
Carrying forward r+

Comment 6

[Douglas Crosher [:dougc]]

See errors compiling with the above patch:

js/src/jit/MIR.h:3513:31: error: expected template-name before ‘<’ token
js/src/jit/MIR.h:3513:31: error: expected ‘{’ before ‘<’ token
js/src/jit/MIR.h:3513:31: error: expected unqualified-id before ‘<’ token
In file included from js/src/jit/MIRGenerator.h:13:0,
                 from js/src/jit/MIRGraph.h:16,
                 from js/src/vm/ForkJoin.cpp:22:

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fixed nits

RuntimePolicy was renamed into FloatingPointPolicy in bug 913282, which hasn't landed yet.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Douglas, I checked on ARM qemu and it seems to work correctly. Could you test on some real ARM devices, please? It's a huge win and that would be nice to get it landed.

Comment 9

[Douglas Crosher [:dougc]]

(In reply to Benjamin Bouvier [:bbouvier] (not online until 10/16) from comment #8)
> Douglas, I checked on ARM qemu and it seems to work correctly. Could you
> test on some real ARM devices, please? It's a huge win and that would be
> nice to get it landed.

This patch seems to have been included in the fuzzing patch in bug 913282
and testing is so far positive.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: rebased

Douglas, thanks for your feedback. I re-checked locally and tests still pass. I think this patch can now land.

Comment 11

[Guilherme Lima]

This bug landed with the wrong number (918613)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Indeed. Ben, try to be more careful in the future :)

https://hg.mozilla.org/mozilla-central/rev/e1226725f674

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Oops! Sorry about that, I'll double check every time from now.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bedc0859755 (tests)

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6bedc0859755