Add MLoadUnboxedObjectOrNull::foldsTo

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8728828 [details] [diff] [review]
Patch

      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 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

3 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)
(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

3 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 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b7481e9a32d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 9

3 years ago
Created attachment 8732830 [details] [diff] [review]
Patch

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

3 years ago
Attachment #8732830 - Flags: review?(jdemooij) → review+
You need to log in before you can comment on or make changes to this bug.