Float32 microbenchmark regression on awfy on 2014-12-30

VERIFIED FIXED in Firefox 38

Status

()

Core
JavaScript Engine: JIT
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: h4writer)

Tracking

Trunk
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
http://arewefastyet.com/#machine=28&view=single&suite=misc&subtest=f32-exp&start=1419935222&end=1419955969

Between 36 and 50% of regression, x86 and x64.  I'll look at this, unless Hannes beats me to it :)
Flags: needinfo?(benj)
(Assignee)

Comment 1

3 years ago
Created attachment 8559066 [details] [diff] [review]
Patch

We should specialize based on TI (by adding a typebarrier) when the MIRType is MIRType_Float and TI says the output is MIRType_Double. MIRType_Float is more narrow than MIRType_Double
Assignee: nobody → hv1989
Attachment #8559066 - Flags: review?(benj)
(Assignee)

Comment 2

3 years ago
*shouldN'T
(Reporter)

Comment 3

3 years ago
Comment on attachment 8559066 [details] [diff] [review]
Patch

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

Thanks for jumping in! Does it fix the regression?

::: js/src/jit/IonBuilder.cpp
@@ +4505,5 @@
> +        MIRType observedType = types->getKnownMIRType();
> +
> +        // Don't specialize if type is MIRType_Float32 and TI reports
> +        // MIRType_Double. Float is more specific than double.
> +        if (observedType == MIRType_Double && rdef->type() == MIRType_Float32)

In some cases, it could be that the observed type is even MIRType_Int32 (e.g. for (var i = 0; i < 1000; i++) Math.fround(i);), could we make the first part of the condition IsNumberType(observedType), instead?
Attachment #8559066 - Flags: review?(benj) → review+
(Reporter)

Updated

3 years ago
Flags: needinfo?(benj)
(Assignee)

Comment 5

3 years ago
Answer to the question:
> <h4writer> bbouvier, about the suggestion. That would be counter-productive. MIRType_Int32 is more specialized than MIRType_Float32. So in that case it is better to specialize ;)
> * Jesse has quit (Client exited)
> <bbouvier> h4writer: yes, but what if we've used Math.fround? (which creates the MIRType_Float32 in the first place)
> <bbouvier> h4writer: TI will say it only saw Int32, but the real type is and should actually be Float32, no?
> <h4writer> bbouvier, TI will indeed say it saw Int32, which is correct. If we can optimize all uses of the result of Math.fround to be int32 that would be much faster than using float arithmetics
> <h4writer> bbouvier, as soon as we hit a float32, TI will say double
> <h4writer> bbouvier, so Math.fround means we want to round to float32. doesn't imply we definitely need to use float32 
> <bbouvier> h4writer: okay, thanks for explaining :)

Secondly:
Yes it fixes the regression ;)
https://hg.mozilla.org/mozilla-central/rev/05ea7a0bb09f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Reporter)

Comment 7

3 years ago
I just verified that the regression has gone, cheers!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.