Closed
Bug 782590
Opened 12 years ago
Closed 12 years ago
Imprecise type information on 3d-cube
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: bhackett1024)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
6.65 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Comment 2•12 years ago
|
||
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).
Attachment #651737 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 3•12 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. https://hg.mozilla.org/projects/ionmonkey/rev/599a761c17fe
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•