795803 - IonMonkey: Enable ICing of JSNative and PropertyOp getters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Branching this out from from bug 786126 to make it easier to track.  Enable the generation of IC stubs for calling of PropertyOp getters.

Comment 1

[Kannan Vijayan [:djvj]]

Last try push: https://tbpl.mozilla.org/?tree=Try&rev=96692d56104e

Comment 2

[Kannan Vijayan [:djvj]]

Bug found: bad ordering of checks in TryAttachNativeStub - some checks on the resulting holder object occurred before the null check on the result of lookupProperty (indicating property not found).

Fixed, and new try build is:

https://tbpl.mozilla.org/?tree=Try&rev=e4bb51cbb3e0

The orange in M(1) is a crash inside GetPcScript, when it tries to walk up the stack frame to get script info and fails.  This is because in |buildOOLFakeExitFrame| we use the masm's |framePushed()| value to create the exit frame descriptor.  This is supposed to describe the frame size.  However, the masm instance used in the OOL stub generator is completely disconnected from the masm instance used to generate the mainline code, which means their |framePushed()| won't be synced.

Need to thread the framePushed() into the IC during Ion code generation, and use that to initialize the OOL masm's framePushed().

Comment 3

[Kannan Vijayan [:djvj]]

After fixing the frameSize issue in OOL exit frames, this was the try result:

https://tbpl.mozilla.org/?tree=Try&rev=2815b2d60dc1

The error in M(3) was that I was passing the PropertyName directly into the getter without checking for a tinyId/shortId in those cases where those are necessary.

Found and fixed that today, new try is at:

https://tbpl.mozilla.org/?tree=Try&rev=d57427546d22

Comment 4

[Kannan Vijayan [:djvj]]

Fixed up the 32-bit build issues, rebased to tip, minor changes to ensure that the exit frame from stub code contained a pointer to the stub IonCode so it would be properly rooted, and merged the jsnative and property-op patches into one patch.  New try:

https://tbpl.mozilla.org/?tree=Try&rev=640ff09a3a11

Comment 5

[Kannan Vijayan [:djvj]]

There was a silly assertion issue introduced in those fixes above which would have led to a lot of try failures.  The |pushWithPatch| doesn't update |framePushed| on the macro assembler (as opposed to all the calls to |Push|), which was being checked for consistency with an assert in the code.  Changed code to manually adjust |framePushed| For the pushWithPatch.

New try is at:
https://tbpl.mozilla.org/?tree=Try&rev=32d7866c00f2

Comment 6

[Kannan Vijayan [:djvj]]

Changed title to reflect the fact that the patch supports both JSNative and PropertyOp getters.

Comment 7

[Kannan Vijayan [:djvj]]

The oranges in the previous try are from the tip I was working off of, and relate to memory leaks that are unrelated to my patch (checked on tbpl).

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3d21b84a63

See bug 786126 for reviews.  Jan reviewed two separate patches which I've consolidated into one for the push since they sit right on top of each other in the code.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fa3d21b84a63