The default bug view has changed. See this FAQ.

TI+JM: add LICM for typed arrays

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
+++ This bug was initially created as a clone of Bug #663485 +++

I had a WIP-patch for this in bug 663485, but filing a separate bug so that we can close the other bug and focus on the LICM part here.
(Assignee)

Comment 1

6 years ago
Created attachment 548276 [details] [diff] [review]
Hoist length

I'm trying smaller patches in this bug. This hoists arr.length for typed arrays. The patch adds an arrayKind member to invariant entries, this seemed the best approach: especially for hoisted bounds checks (not in this patch) we can share most code for dense/typed arrays.
Attachment #548276 - Flags: review?(bhackett1024)
Comment on attachment 548276 [details] [diff] [review]
Hoist length

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

There are several places missed here where we scan the existing invariants and do tests based on arraySlot (entryRedundant, addHoistedCheck, invariantArraySlots, ...).  Can you either have these check the arrayKind too, or just use separate kinds for the hoisted typed array length/slots?  I would need to see the bounds check hoisting patch, but it seems to me like we should be able to reuse the code whether or not different kinds are used.  Either way, InvariantEntry is definitely getting complicated enough that it should have better abstraction (shouldn't need fixing for this bug).
(Assignee)

Comment 3

6 years ago
Comment on attachment 548276 [details] [diff] [review]
Hoist length

(In reply to comment #2)
> There are several places missed here where we scan the existing invariants
> and do tests based on arraySlot (entryRedundant, addHoistedCheck,
> invariantArraySlots, ...).  Can you either have these check the arrayKind
> too, or just use separate kinds for the hoisted typed array length/slots?

This patch only adds the arrayKind field to the .array struct, not the .check struct which is used by these functions.

> I would need to see the bounds check hoisting patch, but it seems to me like
> we should be able to reuse the code whether or not different kinds are used.

My initial patch renamed INVARIANT_SLOTS to INVARIANT_DENSE_SLOTS and added INVARIANT_TYPED_SLOTS. Same for *LENGTH and BOUNDS_CHECK. The reason was that the only difference between a dense/typed array BOUNDS_CHECK is the code generation part but maybe it's better to be explicit about it and use another kind, will investigate.
Attachment #548276 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

6 years ago
Created attachment 548873 [details] [diff] [review]
Patch v2

Decided to use a single patch after all. I renamed INVARIANT_LENGTH to DENSE_ARRAY_LENGTH and added TYPED_ARRAY_LENGTH. This seemed more readable than INVARIANT_TYPED_LENGTH. Same for BOUNDS_CHECK and *SLOTS.
Attachment #548276 - Attachment is obsolete: true
Attachment #548873 - Flags: review?(bhackett1024)
Comment on attachment 548873 [details] [diff] [review]
Patch v2

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

::: js/src/methodjit/LoopState.cpp
@@ +746,5 @@
>  
>      for (unsigned i = 0; i < invariantEntries.length(); i++) {
>          InvariantEntry &entry = invariantEntries[i];
> +        if ((entry.kind == InvariantEntry::DENSE_ARRAY_SLOTS ||
> +             entry.kind == InvariantEntry::TYPED_ARRAY_SLOTS) &&

It would be good to have a short comment noting why no arrayKind check is necessary.

@@ +824,5 @@
>  
>      for (unsigned i = 0; i < invariantEntries.length(); i++) {
>          InvariantEntry &entry = invariantEntries[i];
> +        if ((entry.kind == InvariantEntry::DENSE_ARRAY_LENGTH ||
> +             entry.kind == InvariantEntry::TYPED_ARRAY_LENGTH) &&

Ditto
Attachment #548873 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

6 years ago
Pushed with comments added:

http://hg.mozilla.org/projects/jaegermonkey/rev/235a8bfe2665
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.