Last Comment Bug 782590 - Imprecise type information on 3d-cube
: Imprecise type information on 3d-cube
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
: general
:
Mentors:
Depends on:
Blocks: IonSpeed
  Show dependency treegraph
 
Reported: 2012-08-14 03:52 PDT by Jan de Mooij [:jandem]
Modified: 2012-08-16 06:20 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.65 KB, patch)
2012-08-14 06:07 PDT, Brian Hackett (:bhackett)
jdemooij: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-08-14 03:52:05 PDT
3d-cube allocates small arrays for matrices/vactors in a bunch of different places. For instance:

var R = [
  [1,0,0,0],
  [0,Cos,-Sin,0],
  [0,Sin,Cos,0],
  [0,0,0,1]
];

All these "inner" arrays get their own TypeObject. This leads to imprecision or large type sets in a few places where we access these arrays, like the MMulti function:

#7:00123: 103  getelem
  typeset 14: object[20] [0x1d00720] [0x1d00760] [0x1d00a40] [0x1d00a00] [0x1d00680] [0x1d006c0] [0x1d00900] [0x1d009c0] [0x1d00980] [0x1d008c0] [0x1d00740] [0x1d00780] [0x1d
00a60] [0x1d00a20] [0x1d006a0] [0x1d006e0] [0x1d00920] [0x1d009a0] [0x1d00960] [0x1d008e0]
  type 0: object

And another access where we don't have a precise result type:

#7:00139: 103  getarg 0
  type 0: object[5] [0x1d00700] [0x1d00660] [0x1d008a0] [0x1d00940] [0x1d009e0]
#7:00142: 103  getlocal 1
  type 0: int
#7:00145: 103  getelem
  typeset 18: object
  type 0: object

There are also a lot of barriers:

#7:00132: 103  getelem
  typeset 16: object[4] [0x1d00240] [0x1d002e0] [0x1d00380] [0x1d00760]
  type 0: object[4] [0x1d002e0] [0x1d00240] [0x1d00380] [0x1d00760]
  barrier: [0x1d00720] [0x1d00740] [0x1d00780] [0x1d00340] [0x1d00360] [0x1d003a0] [0x1d00200] [0x1d00220] [0x1d00260] [0x1d002a0] [0x1d002c0] [0x1d00300]

I verified this also happens on mozilla-inbound, but it's worse for IonMonkey because we have to use two GetElement ICs in this function and the ICs prevent hoisting.

Brian, is there anything we can do here, like using fewer TypeObject's?
Comment 1 Brian Hackett (:bhackett) 2012-08-14 06:07:30 PDT
Created attachment 651737 [details] [diff] [review]
patch

Yeah, TI's handling of multidimensional array initializers has always been annoying.  I think this used to be hard to fix, but that's not the case anymore.  This should give the same type to each element of the outer array.
Comment 2 Jan de Mooij [:jandem] 2012-08-15 06:04:18 PDT
Comment on attachment 651737 [details] [diff] [review]
patch

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

Thanks for fixing this.

::: js/src/jsinfer.cpp
@@ +1896,5 @@
> +     * If this is an array initializer nested in another array initializer,
> +     * try to reuse the type objects from earlier elements to avoid
> +     * distinguishing elements of the outer array unnecessarily.
> +     */
> +    do {

Would be good to move the loop body to its own function so that we can drop the do { } while(0) loop and keep addAllocationSiteTypeObject shorter and more readable.

@@ +1945,5 @@
> +
> +        /* Find the start of the previous initializer. */
> +        jsbytecode *previnit;
> +        for (previnit = last; previnit; previnit = PreviousOpcode(script, previnit)) {
> +            if (*previnit == JSOP_NEWINIT || *previnit == JSOP_NEWARRAY || *previnit == JSOP_NEWOBJECT)

This may stop at an array initializer nested inside the previous one. Is it okay to reuse its TypeObject? If not, this loop could just stop when it sees a JSOP_ENDINIT (other than the expected one of course, where the loop starts).
Comment 3 Brian Hackett (:bhackett) 2012-08-16 06:20:03 PDT
Good catch on the 3+ dimension array initializers, fixed so that the second dimension get the same type as each other by using a counter.  The reuse in 3+ dimensional arrays in the first patch is ok to do, but precision will be degraded (contents of 2nd and 3rd dimension arrays will be conflated).

Pushed straight to IonMonkey repo, by request.

https://hg.mozilla.org/projects/ionmonkey/rev/599a761c17fe

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