Closed
Bug 1255316
Opened 8 years ago
Closed 8 years ago
Add MLoadUnboxedObjectOrNull::foldsTo
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: h4writer, Assigned: h4writer)
Details
Attachments
(2 files)
1.90 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
834 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
1 Richards: 27564 29 MLoadUnboxedObjectOrNull::foldsTo 1 DeltaBlue: 55890 1 Crypto: 30049 2 done 1 RayTrace: 113810 1 EarleyBoyer: 35785 1 RegExp: 2526 1 Splay: 16306 1 SplayLatency: 6774 1 NavierStokes: 30627 1 PdfJS: 6027 1 Mandreel: 5725 1 MandreelLatency: 13809 4 MLoadUnboxedObjectOrNull::foldsTo 1 Gameboy: 46210 1 CodeLoad: 17912 18 MLoadUnboxedObjectOrNull::foldsTo 1 Box2D: 10428 1 zlib: 68805 49 MLoadUnboxedObjectOrNull::foldsTo 1 Typescript: 20355 1 ---- 1 Score (version 9): 19598 A couple of places where this helps.
Assignee: nobody → hv1989
Attachment #8728828 -
Flags: review?(jdemooij)
Comment 2•8 years ago
|
||
Comment on attachment 8728828 [details] [diff] [review] Patch Review of attachment 8728828 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.cpp @@ +4731,5 @@ > return foldsToStoredValue(alloc, store->value()); > } > > +MDefinition* > +MLoadUnboxedObjectOrNull::foldsTo(TempAllocator& alloc) MLoadUnboxedObjectOrNull has offsetAdjustment_ and nullBehavior_ fields. Shouldn't we check offsetAdjustment_ is the same as the store's adjustment (and also for MLoadElement) and check nullBehavior_ != BailOnNull or something?
Attachment #8728828 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8728828 [details] [diff] [review] Patch Review of attachment 8728828 [details] [diff] [review]: ----------------------------------------------------------------- The fields don't need to get included during foldsTo. Since we are on the same "elements", those should be the same! I can assert that they are the same?
Attachment #8728828 -
Flags: review?(jdemooij)
Comment 4•8 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #3) > The fields don't need to get included during foldsTo. Since we are on the > same "elements", those should be the same! > I can assert that they are the same? What about the nullBehavior_/BailOnNull thing?
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #4) > (In reply to Hannes Verschore [:h4writer] from comment #3) > > The fields don't need to get included during foldsTo. Since we are on the > > same "elements", those should be the same! > > I can assert that they are the same? > > What about the nullBehavior_/BailOnNull thing? Good question. First/second/third and fourth look again convinced me it is not needed. Though every time for other reasons and not obvious. My last reasoning is: When BailOnNull is set, the type of the load is MIRType_Object. When replacing with the value of a store, the 'foldsToStoredValue' checks if the types are correct. If the value of the store would be MIRType_Null or include MIRType_Null, foldsToStoredValue will not fold the load with the store. Therefore we can never have a load being folded to include Null suddenly.
Comment 6•8 years ago
|
||
Comment on attachment 8728828 [details] [diff] [review] Patch Review of attachment 8728828 [details] [diff] [review]: ----------------------------------------------------------------- OK, thanks for looking into that. We should definitely add asserts for these two fields, though :)
Attachment #8728828 -
Flags: review?(jdemooij) → review+
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1b7481e9a32d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 9•8 years ago
|
||
Seems my hypothesis on offsetAdjustment was wrong. IIUC the type is not solely defined by the object, but also guessed by the output type of the load.
Attachment #8732830 -
Flags: review?(jdemooij)
Updated•8 years ago
|
Attachment #8732830 -
Flags: review?(jdemooij) → review+
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d2eaee9d7294
You need to log in
before you can comment on or make changes to this bug.
Description
•