1129377 - Float32 microbenchmark regression on awfy on 2014-12-30

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

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 :)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: 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

Comment 2

[Hannes Verschore [:h4writer]]

*shouldN'T

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 4

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05ea7a0bb09f

Comment 5

[Hannes Verschore [:h4writer]]

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 ;)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05ea7a0bb09f

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

I just verified that the regression has gone, cheers!