838991 - StackTypeSet::convertDoubleElements preventing dense arrays from being compiled as such in Ion

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Attachment: microbenchmark

The current logic on |StackTypeSet::convertDoubleElements| returns |AmbiguousDoubleConversion| for a type set that includes a mix between packed arrays with an element type set of {int} and packed arrays with an element typeset of {int,float}. The {int} packed array will set |dontConvert = true| via |!types->hasType(Types::DoubleType())|, and the {int,float} packed array will set |maybeConvert = true|.

This causes a getelem IC, or worse, a VM call.

There's a ParallelArray benchmark that has a loop similar to the |sum| loop in the attached microbenchmark.

Benchmark before patch:
  % time: 57

Benchmark after patch:
  % time: 32

Comment 1

[Shu-yu Guo [:shu]]

Attachment: patch

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 711196 [details] [diff] [review]
patch

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

::: js/src/jsinfer.cpp
@@ +2047,5 @@
> +            if (!types->hasType(Type::Int32Type()))
> +                dontConvert = true;
> +            alwaysConvert = false;
> +            continue;
> +        }

This logic will allow writing doubles to an array that only contains int32s, which will both break the type information for the array and unnecessarily deoptimize it.

Can you post the benchmark and point out where the problem is at?  This has to be a setelem rather than a getelem as an AmbiguousDoubleConversion on getelem just means we can't do an optimized double load.

I think the only way to fix this is a new MIR/LIR setelem node that checks for double arrays and then either writes its int32 input or converts the input to a double first.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> This logic will allow writing doubles to an array that only contains int32s,
> which will both break the type information for the array and unnecessarily
> deoptimize it.
> 

I see.

> Can you post the benchmark and point out where the problem is at?  This has
> to be a setelem rather than a getelem as an AmbiguousDoubleConversion on
> getelem just means we can't do an optimized double load.

The benchmark is already attached. The problem is that all the reads from a[i+j] gets compiled as GetElemCache, as |TypeInferenceOracle::elementReadIsDenseNative| returns true only if convertDoubleElements returns something other than AmbiguousDoubleConversion.

Among other things, GetElemCache isn't thread-safe.

> 
> I think the only way to fix this is a new MIR/LIR setelem node that checks
> for double arrays and then either writes its int32 input or converts the
> input to a double first.

Comment 4

[Shu-yu Guo [:shu]]

I was looking at the wrong tree. This problem is local to the ParallelArray branch.