898021 - Truncate values written to integer typed arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch.txt

Integer typed arrays convert their value inputs to integers before writing per ToInt32.  This truncation is the same as occurs for bitwise operations and can be incorporated into the range analysis for truncating more ops.  Making this change allows some typed array loads in asm.js benchmarks to be done infallibly, and improves asmjs-ubench-fannkuch --no-asmjs by about 2.5% on x86, closing more than half the gap with --asmjs.

This patch doesn't deal with the similar problem faced by loading and then storing elements to float typed arrays, as the range analysis only deals with integer truncation.  Will fix that in the future when I see a test affected by that, either by extending the range analysis or adding some code elsewhere.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 781057 [details] [diff] [review]
patch.txt

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

::: js/src/ion/RangeAnalysis.cpp
@@ +1808,5 @@
>  
> +bool
> +MStoreTypedArrayElement::isOperandTruncated(size_t index) const
> +{
> +    return index == 2 && !isFloatArray();

Nit: you can do the same for StoreTypedArrayElementHole, maybe with a comment explaining that typed array OOB writes are always no-ops (see codegen).

@@ +1816,5 @@
> +MStoreTypedArrayElementStatic::isOperandTruncated(size_t index) const
> +{
> +    return index == 1 &&
> +        viewType() != ArrayBufferView::TYPE_FLOAT32 &&
> +        viewType() != ArrayBufferView::TYPE_FLOAT64;

Nit: for consistency, it would be nice to add an isFloatArray() to StoreTypedArrayElementStatic

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Nit: you can do the same for StoreTypedArrayElementHole, ..

Just to be sure, not exactly the same: the index should be 3 instead of 2 there.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01bf6add1d1f

Comment 4

[Daniel Holbert [:dholbert]]

This caused a bunch of new build-warning-spam -- in my local mozilla-inbound build, every .cpp file that #includes MIR.h now prints out this warning, about a line of code that was added in this bug's commit:
{
js/src/ion/MIR.h: In member function 'bool js::ion::MStoreTypedArrayElementStatic::isFloatArray() const':

js/src/ion/MIR.h:5328:49: warning: comparison between 'enum js::ArrayBufferView::ViewType' and 'enum js::TypedArrayObject::<anonymous>' [-Wenum-compare]

          return (viewType() == TypedArrayObject::TYPE_FLOAT32 ||
                                                  ^
}

Our TBPL builders are hitting this, too -- e.g. here's a build from the cset where this landed, with a bunch of instances of this warning:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=25788733&tree=Mozilla-Inbound

Comment 5

[Daniel Holbert [:dholbert]]

The warning is a real bug (i.e. we are indeed comparing enums of two different types). viewType returns type "ViewType", defined here:
http://mxr.mozilla.org/mozilla-central/source/js/src/jsfriendapi.h?rev=bbf37166d07c&mark=974-981#974

...whereas TypedArrayObject::TYPE_FLOAT32 is from a different enum, defined here:
http://mxr.mozilla.org/mozilla-central/source/js/src/vm/TypedArrayObject.h?rev=2aa4435cd798&mark=284-291#273

I think this needs s/TypedArrayObject/ArrayBufferView/ on that line, or something like that...

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: followup to fix -Wenum-compare build warning

This fixes the warning for me. (s/TypedArrayObject/ArrayBufferView/)

Comment 7

[Daniel Holbert [:dholbert]]

Landed followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a41bb1a3c9d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01bf6add1d1f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a41bb1a3c9d