1193112 - Unconditional Ion Bailouts caused by Float32 types.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Michael Bebenita [:mbx]]

The following code reads a float 32 value and calls the self hosted isNaN() function. For some reason, this causes IonBuilder to generate an unconditional bailout that is taken every time the f() function is called.

var buf = new ArrayBuffer(8);
var i32 = new Int32Array(buf);
var f32 = new Float32Array(buf);

function f(a) {
  i32[0] = a;
  isNaN(f32[0]); 
}

for (var i = 0; i < 10000000; i++) {
  f(1065353216);
}

Comment 1

[Shu-yu Guo [:shu]]

So the MBail that's inserted by the typeset filtering stuff is not an invalidation bailout. It'll just bail out without recompiling the IonScript, forever.

Comment 2

[Shu-yu Guo [:shu]]

Hannes, I think you reviewed the typeset filtering stuff?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: filtertypeset-nofloat32.patch

It's a rather simple fix. MFilterTypeSet's type policy checks that the input and output types are the same. But it determines the output type by looking at the type set (look at MFilterTypeSet's ctor), and type inference doesn't know about float32 and only sees doubles! So the inputType == outputType check fails, and and as the inputType isn't Value, a bail is unconditionally inserted.

I see two ways to fix this:
1) either special-case the detection of Float32 in the first check (that is, check if inputType is Float32 and outputType is Double)
2) or just force ToDouble coercion of the input, if it's a float32

(well, there's actually a third way: teach type inference about float32, but it's beyond the scope of this bug)

1) sounds wrong, as lowering for FilterTypeSet just consists in redefining the input (that would mean we'd have an input which is a float32, while the output is a double, so there'd be a type discrepancy here). As a matter of fact, just ensure the input isn't a double.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Implement float32 to filtertypeset

I really would want that MFilterTypeSet is mostly a NOP and doesn't cause worse performance. I foresee myself investigating a slowdown caused by having Float32 not implemented for MFilterTypeSet in the future. Making sure future Hannes doesn't need to spend a day investigating it by fixing it now ;).

Note: I'm a bit fuzzy on every Float32 corner-case. But this fix the test locally ...

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8646827 [details] [diff] [review]
Implement float32 to filtertypeset

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

Ha, thanks for having me thinking about all the Float32 checks again :) I really should write a small blog post to explain the algorithm and what these functions mean, because it's really unclear.

Please double check me, but I think the FilterTypeSet should behave like a Phi that has only one input, w.r.t float32. More comments inline.

::: js/src/jit/MIR.cpp
@@ +2249,5 @@
> +    if (in->type() != MIRType_Float32)
> +        return;
> +
> +    if (!in->canProduceFloat32())
> +        ConvertDefinitionToDouble<0>(alloc, in, this);

So you might convert the input to a double while setting the output to be a float32? There's a type discrepancy here. I think you've forgot to "return" after this statement.

Thinking about it, I guess you can just remove this check and the if body: for instance, a float32 MAdd should be able to flow into a FilterTypeSet and stay a float32.

::: js/src/jit/MIR.h
@@ +12260,5 @@
>      }
>      void computeRange(TempAllocator& alloc) override;
> +
> +    bool isFloat32Commutative() const override { return true; }
> +    bool canProduceFloat32() const override { return type() == MIRType_Float32; }

The type will be set to Float32 in trySpecializeFloat32 only, which happens *after* we've computed the producer capacity of phis. So if a FilterTypeSet that has a float32 input flows into a phi which only has float32 inputs, this phi won't be marked as producing float32!
I think canProduceFloat32() should just return input->canProduceFloat32(), which makes sense as it's supposed to be a no-op.

@@ +12261,5 @@
>      void computeRange(TempAllocator& alloc) override;
> +
> +    bool isFloat32Commutative() const override { return true; }
> +    bool canProduceFloat32() const override { return type() == MIRType_Float32; }
> +    bool canConsumeFloat32(MUse* use) const override { return true; }

canConsumeFloat32 shouldn't return true all the time. Otherwise, the + operations in e.g. FilterTypeSet(f32(x) + f32(y)) would be done as a float32+, while there's no reason it should be. (Note that it can or cannot be done as a float32+, depending on what's using the FilterTypeSet).

Although, if all FilterTypeSet's uses are consumers, then the FilterTypeSet can be a consumer as well.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Implement float32 to filtertypeset

Updated with the remarks.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8647412 [details] [diff] [review]
Implement float32 to filtertypeset

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

Looks good, thank you! I guess it solves the original issue. If you're willing to, feel free to add a few tests in jit-tests/tests/ion/testFloat32.js which check that a FilterTypeSet has the float32 type in some cases, and doesn't have it in other cases (using the assertFloat32(variable, true|false) function).

::: js/src/jit/MIR.h
@@ +12269,5 @@
> +        // of this operand to determine if it can consume a float32.
> +        bool canConsumeFloat32 = true;
> +        for (MUseDefIterator use(this); canConsumeFloat32 && use; use++) {
> +            MDefinition* usedef = use.def();
> +            canConsumeFloat32 &= usedef->canConsumeFloat32(use.use());

In MIR.cpp, you have CheckUsesAreFloat32Consumers, which does exactly that.

Comment 8

[Pulsebot]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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