Bug 173687 – ValueRep(DoubleRep(@v)) can not simply convert to @v

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

consider this IR:

 block#x
  p: Phi() // int32 and double flows into this phi from various control flow
  d: DoubleRep(@p)
  some uses of @d here
  v: ValueRep(DoubleRepUse:@d)
  a: NewArrayWithSize(Int32:@v)
  some more nodes here ...

Because the flow of ValueRep(DoubleRep(@p)) will not produce an Int32, AI proves that the Int32 check will fail. Constant folding phase removes all nodes after @a and inserts an Unreachable after the NewArrayWithSize node.

The IR then looks like this:
block#x
  p: Phi() // int32 and double flows into this phi from various control flow
  d: DoubleRep(@p)
  some uses of @d here
  v: ValueRep(DoubleRepUse:@d)
  a: NewArrayWithSize(Int32:@v)
  Unreachable

However, there is a strength reduction rule that tries eliminate redundant conversions. It then converts the program to:
block#x
  p: Phi() // int32 and double flows into this phi from various control flow
  d: DoubleRep(@p)
  some uses of @d here
  a: NewArrayWithSize(Int32:@p)
  Unreachable

At runtime, @p will actually be an Int32, so @a will not OSR exit, and we’ll crash. I believe this strength reduction rule is wrong, since it produces different output than if we ran the program without it. However, we clearly want to keep some optimizations for not performing redundant conversions. I think there are conditions under which we can perform this optimization. For example, if @p definitely produces a Double, I think this optimization would be sound.

[Mark Lam]

<rdar://problem/32855563>

[Saam Barati]

This program crashes JSC:

```
let a1 = [1,2,3,4];
delete a1[2];

let a2 = [];
a2.length = null;

let thingy = {length: 2**55, __proto__: []};

let arrays = [
    [10, {}, 20.5, 7821],
    a2,
    [10.2, 122.55, 10.22, 12.44],
    a2,
    [10,,10,,,20,,,,,50],
    a1,
    a2,
];

noInline(Array.prototype.map);

let funcs = [
    (x) => x,
    (x) => x,
    (x) => x,
    (x) => x,
    (x) => x,
    (x) => x,
    (x) => x,
    (x) => x,
    (x) => x,
];

function loop(x) {
    for (let i = 0; i < x; ++i) {
        try {
            if (i > 0 && (i % 1000) === 0)
                thingy.map(x => x)
            arrays[i % arrays.length].map(funcs[i % funcs.length]);
        } catch(e) {
        }
    }
}

loop(100000);
```

[Saam Barati]

Created attachment 313650 [details]
patch

[Mark Lam]

Comment on attachment 313650 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313650&action=review

r=me

> Source/JavaScriptCore/ChangeLog:41
> +        and weâll crash. This patch removes this strength reduction rule since it

Please remove the non-ascii char in "we'll".

[Saam Barati]

Created attachment 313651 [details]
patch for landing

[Keith Miller]

Comment on attachment 313651 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=313651&action=review

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:221
>              // This short-circuits circuitous conversions, like ValueRep(DoubleRep(value)) or
>              // even more complicated things. Like, it can handle a beast like
>              // ValueRep(DoubleRep(Int52Rep(value))).

Seems like you should update this comment. It seems quite wrong...

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/218728/webkit
and fixed the comment.