Bug 200018 - Sometimes we miss removable CheckInBounds
Summary: Sometimes we miss removable CheckInBounds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-22 15:54 PDT by Justin Michaud
Modified: 2019-07-23 10:25 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.24 KB, patch)
2019-07-22 16:00 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (2.71 KB, patch)
2019-07-22 16:40 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2019-07-22 16:56 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2019-07-22 15:54:51 PDT
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.
Comment 1 Justin Michaud 2019-07-22 16:00:23 PDT
Created attachment 374647 [details]
Patch
Comment 2 Justin Michaud 2019-07-22 16:40:07 PDT
Created attachment 374652 [details]
Patch
Comment 3 Saam Barati 2019-07-22 16:47:51 PDT
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.
Comment 4 Saam Barati 2019-07-22 16:48:24 PDT
Comment on attachment 374652 [details]
Patch

Can you also add a microbenchmark if one doesn't already exist
Comment 5 Justin Michaud 2019-07-22 16:56:51 PDT
Created attachment 374654 [details]
Patch
Comment 6 WebKit Commit Bot 2019-07-23 10:24:20 PDT
Comment on attachment 374654 [details]
Patch

Clearing flags on attachment: 374654

Committed r247724: <https://trac.webkit.org/changeset/247724>
Comment 7 WebKit Commit Bot 2019-07-23 10:24:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-07-23 10:25:18 PDT
<rdar://problem/53452180>