Closed Bug 918163 Opened 6 years ago Closed 6 years ago

IonMonkey: specialize MMathFunction for Float32

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

So that Math.fround(cos(Math.fround(1)) uses cosf and not cos.
OOC, will the float analysis recognize literals that are exactly representable as floats like 1 or 1.5 as float producers?
(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.
Attached patch Some maths functions (obsolete) — Splinter Review
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.
Attachment #807555 - Flags: review?(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.
Attachment #807555 - Flags: review?(sstangl) → review+
Attached patch fixed nits (obsolete) — Splinter Review
Thanks for the quick review!
Carrying forward r+
Attachment #807555 - Attachment is obsolete: true
Attachment #808004 - Flags: review+
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:
Attached patch fixed nits (obsolete) — Splinter Review
RuntimePolicy was renamed into FloatingPointPolicy in bug 913282, which hasn't landed yet.
Attachment #808004 - Attachment is obsolete: true
Attachment #808818 - Flags: review+
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.
Flags: needinfo?(dtc-moz)
(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.
Flags: needinfo?(dtc-moz)
Blocks: 900120
Attached patch rebasedSplinter Review
Douglas, thanks for your feedback. I re-checked locally and tests still pass. I think this patch can now land.
Attachment #808818 - Attachment is obsolete: true
Attachment #818273 - Flags: review+
Keywords: checkin-needed
This bug landed with the wrong number (918613)
Indeed. Ben, try to be more careful in the future :)

https://hg.mozilla.org/mozilla-central/rev/e1226725f674
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Oops! Sorry about that, I'll double check every time from now.
Depends on: 941381
Depends on: 969203
You need to log in before you can comment on or make changes to this bug.