Last Comment Bug 737783 - IonMonkey: GETELEM: Fast path for typed arrays
: IonMonkey: GETELEM: Fast path for typed arrays
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 735408
  Show dependency treegraph
Reported: 2012-03-21 02:29 PDT by Jan de Mooij [:jandem]
Modified: 2012-03-23 06:47 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (63.30 KB, patch)
2012-03-21 02:54 PDT, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2012-03-21 02:29:55 PDT

Comment 1 User image Jan de Mooij [:jandem] 2012-03-21 02:54:32 PDT
Created attachment 607885 [details] [diff] [review]

The patch is mostly modeled after the dense array instructions and the JM fast paths. Some notes:

- The *Hole instruction uses an OOL VM call to handle the out-of-bounds case. This should be very uncommon since typed arrays don't have an "initialized length" < length case. Better name welcome, the *Hole suffix matches the dense array *Hole instruction but typed arrays don't have holes - I couldn't think of a better name (with the same length) though.

- Uint32Array is the most complicated array type, because reading from it can result in either an int32 or double value. If the instruction is typed as int32, and we read a double, we have to bailout. Furthermore, we need a temp register for convertUint32ToDouble.

- x86/x64 only for now, Marty is willing to add the ARM macro-assembler methods we need. I will add some temporary #ifdef's to make sure the code compiles on ARM in the meantime.
Comment 2 User image David Anderson [:dvander] 2012-03-21 15:00:18 PDT
Comment on attachment 607885 [details] [diff] [review]

Review of attachment 607885 [details] [diff] [review]:


::: js/src/ion/shared/Assembler-shared.h
@@ +59,5 @@
> +ScaleFromShift(int shift)
> +{
> +    switch (shift) {
> +      case 1:
> +       return TimesOne;

nit: misindent

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +274,5 @@
> +    }
> +    void cvtss2sd(const FloatRegister &src, const FloatRegister &dest) {
> +        masm.cvtss2sd_rr(src.code(), dest.code());
> +    }
> +    void movzbl(const Operand &src, const Register &dest) {

nit: Yesterday I renamed load16 to movzxh or something. I like your convention better, so feel free to rename it.
Comment 3 User image Jan de Mooij [:jandem] 2012-03-23 06:47:15 PDT
Pushed with nits fixed (I used the same names as JSC uses for the instructions):

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