Truncate values written to integer typed arrays

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
mozilla25
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Reporter

Description

6 years ago
Posted patch patch.txtSplinter Review
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.
Attachment #781057 - Flags: review?(jdemooij)
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
Attachment #781057 - Flags: review?(jdemooij) → review+
(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.
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
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...
This fixes the warning for me. (s/TypedArrayObject/ArrayBufferView/)
Attachment #782043 - Flags: review?(bhackett1024)
Reporter

Updated

6 years ago
Attachment #782043 - Flags: review?(bhackett1024) → review+
Landed followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a41bb1a3c9d
OS: Mac OS X → All
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/01bf6add1d1f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.