Imprecise type information on 3d-cube




JavaScript Engine
5 years ago
5 years ago


(Reporter: jandem, Assigned: bhackett)


(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago
3d-cube allocates small arrays for matrices/vactors in a bunch of different places. For instance:

var R = [

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

5 years ago
Created attachment 651737 [details] [diff] [review]

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.
Assignee: general → bhackett1024
Attachment #651737 - Flags: review?(jdemooij)

Comment 2

5 years ago
Comment on attachment 651737 [details] [diff] [review]

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).
Attachment #651737 - Flags: review?(jdemooij) → review+

Comment 3

5 years ago
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.
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.