Bug 200018 – Sometimes we miss removable CheckInBounds

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Michaud]

In the following loop:
+function doTest(arr1) {
+    let sum = 0
+    for (let i=0; i<arr1.length; ++i) {
+        sum += arr1[i]
+    }
+    return sum
+}
+noInline(doTest);
+
+let arr1 = new Int32Array(1000)
+//let arr1 = new Array(1000)
+
+for (let i=0; i<1000; ++i) doTest(arr1)
we should not need to emit a checkInBounds, but we do.

[Justin Michaud]

Created attachment 374647 [details]
Patch

[Justin Michaud]

Created attachment 374652 [details]
Patch

[Saam Barati]

Comment on attachment 374652 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        We failed to remove the CheckInBounds bounds because we did not see that the index was nonnegative. This is because we do not see the relationship between the two
> +        separate zero constants that appear in the IR for the given test case. This patch re-adds the hack to de-duplicate m_zero that was removed in 
> +        <https://trac.webkit.org/changeset/241228/webkit>.

nit: should go below "Reviewed by ..."

Might be worth also opening a bug on not being reliant on the exact node pointer. Or at least understanding why it's ok to rely on the actual node pointer value.

[Saam Barati]

Comment on attachment 374652 [details]
Patch

Can you also add a microbenchmark if one doesn't already exist

[Justin Michaud]

Created attachment 374654 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374654 [details]
Patch

Clearing flags on attachment: 374654

Committed r247724: <https://trac.webkit.org/changeset/247724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53452180>