Closed Bug 671084 Opened 8 years ago Closed 8 years ago

TI+JM: add LICM for typed arrays

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

+++ 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.
Attached patch Hoist length (obsolete) — Splinter Review
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).
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)
Attached patch Patch v2Splinter Review
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+
Pushed with comments added:

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