932767 - Flag code paths tainted by unexpected NaNs to help eliminate canonicalizing floats loaded from typed arrays.

Status: ASSIGNED

(Core :: JavaScript Engine: JIT, enhancement, P5)

Description

[Douglas Crosher [:dougc]]

Floating point values read from typed arrays currently need to be canonicalized to limit the range of NaNs.  See bug 584158 and also bug 584168.

Unfortunately this can have a significant performance impact to some numerical code, and in particular asm.js style code that makes heavy use of typed arrays.

The Odin compiler avoids this canonicalization, and the Chrome JIT also appears to not needed this.

The Ion JIT compiler might flag the data flows of floating point values read from typed arrays and eliminate the canonicalization if the values are written back to typed arrays.

It might also be possible to move canonicalization out of a loop.  For example, if a loop is reducing a collection of floating point values stored in a typed array then it would likely improve performance to canonicalize the reduced value at the end of the loop rather than canonicalizing each value read from the typed array.

Comment 1

[Luke Wagner [:luke]]

This sounds good.  Note: it's very important that on all bail paths, any speculatively-not-canonicalized doubles get canonicalized.

To wit, V8 avoids the canonicalization by not using NaN-boxing to represent their values: their values are represented as pointers to doubles, which are GC-allocated.  In the JIT, of course, they avoid the heap allocation and just pass around unboxed doubles by value.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

That would definitely help some real world JS frameworks, for instance gl-matrix, which perform a lot of loads by function calls.

For a specific benchmark I am looking at, there is up to 50% speedup if I just comment the canonicalizeFloat / canonicalizeDouble call in IonMacroAssembler::loadFromTypedArray.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> For a specific benchmark I am looking at, there is up to 50% speedup if I
> just comment the canonicalizeFloat / canonicalizeDouble call in
> IonMacroAssembler::loadFromTypedArray.

sunfish, can you think of a more efficient implementation of canonicalizeDouble on x86/x64 maybe?

Comment 4

[Dan Gohman [:sunfish]]

You could try moving the NaN constant load out of line (such as with the OutOfLineCodeBase mechanism), so that you can make the non-NaN case a fallthrough. That probably won't completely fix it, but it might help.

Comment 5

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 1: Add tainted floating point types. r=jandem!

Add two new types for floating point values which are possibly tainted by
unexpected NaNs.

The next patches in this series will add the necessary changes to properly
handle the new types.

Comment 6

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 2: Add separate instruction to canonicalize NaN. r=jandem!

Split NaN canonicalization into a separate MIR instruction, so we can make it
optional in later patches of this patch stack.

Comment 7

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 3: Update MIRType checks to use new helpers. r=jandem!

Update MIRType checks to handle the new "tainted" floating point types from
the previous part. Wasm-only code doesn't need any changes, because it doesn't
use the tainted variants.

Changes:

• Replace type == MIRType::Double with IsDoubleType(type).
• Replace type == MIRType::Float32 with IsFloat32Type(type).
• Replace type == otherType with IsEqualType(type, otherType).

Comment 8

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 4: Update switch-statements over MIRType to handle tainted types. r=jandem!

Add case statements to handle the new tainted floating point types.

Comment 9

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 5: Handle tainted types when passed to MDefinition::definitelyType. r=jandem!

Comment 10

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 6: Add type checks for JS::Value compatible types. r=jandem!

Add checks that store-element and store-slot instructions never use tainted inputs.

Comment 11

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 7: Allow to pass tainted types to MToFPInstruction subclasses. r=jandem!

Used in the next parts.

Comment 12

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 8: Update TypePolicy to handle tainted types. r=jandem!

Update type policies to insert MCanonicalizeNaN where necessary.

Comment 13

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 9: Update MacroAssembler to handle tainted types. r=jandem!

Comment 14

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 10: Update GVN for tainted types. r=jandem!

Update foldsTo methods to handle tainted types and the new MCanonicalizeNaN
instruction.

Comment 15

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 11: Update phi specialization for tainted types. r=jandem!

Changes:

• Ensure floating point conversion operations pass the tainted marker.
• Phi specialization uses the non-tainted types for now. Part 12 will add a
  separate pass to compute the correct tainted types.

Comment 16

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 12: Add pass to propagate tainted floating point values. r=jandem!

Add a pass to recursively mark all floating point instructions as tainted when
one of its inputs is tainted. This pass occurs before type conversions are added,
so that type policies can add MCanonicalizeNaN when necessary.

Comment 17

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 13: Add JITOption to allow loading non-canonicalized doubles. r=jandem!

Add an option to enable/disable loading non-canonicalized doubles. When
non-canonical NaNs are allowed, we don't have to emit MCanonicalizeNaN
after typed array loads.

Comment 18

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 14: Add tests. r=jandem!

Comment 19

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 15: Use MCanonicalizeNaN for differential testing. r=jandem!

This will also fix bug 1645795 and bug 1928622.

Drive-by change:

• Move StoreToTypedFloatArray into MacroAssembler::storeToTypedFloatArray to
  match the load methods.

Comment 20

[André Bargull [:anba]]

Attachment: Bug 932767 - Part 16: Rename storeUncanonicalized to just store. r=jandem!