Last Comment Bug 671084 - TI+JM: add LICM for typed arrays
: TI+JM: add LICM for typed arrays
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on:
Blocks: 619423
  Show dependency treegraph
 
Reported: 2011-07-12 14:35 PDT by Jan de Mooij [:jandem]
Modified: 2011-07-28 05:25 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Hoist length (6.75 KB, patch)
2011-07-25 13:50 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch v2 (29.19 KB, patch)
2011-07-27 12:49 PDT, Jan de Mooij [:jandem]
bhackett1024: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-07-12 14:35:11 PDT
+++ 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.
Comment 1 Jan de Mooij [:jandem] 2011-07-25 13:50:11 PDT
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.
Comment 2 Brian Hackett (:bhackett) 2011-07-26 07:07:16 PDT
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 3 Jan de Mooij [:jandem] 2011-07-26 07:54:23 PDT
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.
Comment 4 Jan de Mooij [:jandem] 2011-07-27 12:49:35 PDT
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.
Comment 5 Brian Hackett (:bhackett) 2011-07-27 16:45:59 PDT
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
Comment 6 Jan de Mooij [:jandem] 2011-07-28 05:25:20 PDT
Pushed with comments added:

http://hg.mozilla.org/projects/jaegermonkey/rev/235a8bfe2665

Note You need to log in before you can comment on or make changes to this bug.