Closed Bug 1255316 Opened 6 years ago Closed 6 years ago

Add MLoadUnboxedObjectOrNull::foldsTo

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(2 files)

No description provided.
Attached patch PatchSplinter Review
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)
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?
(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+
https://hg.mozilla.org/mozilla-central/rev/1b7481e9a32d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Attached patch PatchSplinter Review
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)
Attachment #8732830 - Flags: review?(jdemooij) → review+
You need to log in before you can comment on or make changes to this bug.