1214508 - [[SharedStubs] Port GetProp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Part1: Move to getprop stub to shared stub

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Part2: Move helper functions to shared stubs

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Part3: Get stub ready for ion execution

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Part4: Emit getprop shared stub in ion

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Part5: Shared stubs with ion vmcalls require 'masm.framepushed' to be correct

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Part6: Fix ion vmcalls in shared stub

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Part7: Move monitor stub to shared stubs

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Part8: Get monitor stub ready for ion execution

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Part9: Dont sync for decompiler in ion

Comment 10

[Jan de Mooij [:jandem]]

So part 1 doesn't build without part 2? It's nice to merge them in that case to not break bisecting etc.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8674112 [details] [diff] [review]
Part4: Emit getprop shared stub in ion

Review of attachment 8674112 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/JitCompartment.h
@@ +356,5 @@
>      // Keep track of offset into various baseline stubs' code at return
>      // point from called script.
>      void* baselineCallReturnAddrs_[2];
>      void* baselineGetPropReturnAddr_;
> +    void* ionGetPropReturnAddr_;

baselineGetPropReturnAddr_ is used for bailouts from Ion, but the Ion address isn't used right? Let's just not set it in that case.

Comment 12

[Jan de Mooij [:jandem]]

Looks great!

Will there be an option for testing to always use shared caches instead of Ion ICs or inline paths?

Also we can probably hold off on GETELEM and SETELEM, I want to merge them with GETPROP and SETPROP like I'm doing in Ion (and then remove them) so it's probably not worth refactoring them until that :)

Comment 13

[Hannes Verschore [:h4writer]]

Attachment: Part10: Fix pushedFrame state

During my try run I found some fall-out, that could have been prevented with better debug checking. The issue being that we didn't check properly if the 'framePushed_' is correct in stubs.

This patch makes sure we test that after a stub has completed 'framePushed_' is 0. That means we are much stricter than the no negative framePushed_ allowed rule. Which made it easier to find and fix where we didn't count everything correctly.

Stubs have a special construct in which they often have multiple exits and control flows that actually don't fall through. Currently we have tracked the framePushed_ in a sequential structure without looking at the control flow. This was annoying!

As a result I've introduced "FrameAwareLabel". This encapsulates a normal Label, but enforces that the stack is the same on all jump/bind places. As a result more stricter and better checking. Every place where we bind/jump the current framePushed is tested against previous framePushed. If there is a contradiction an assert will be raised.

Currently we have:
masm.j(xxx, FrameAwareLabel*);
masm.jump(FrameAwareLabel*);
masm.bind(FrameAwareLabel*);
masm.bindNoFallThrough(FrameAwareLabel*); // this is when we have no incoming path from above the bind. In debug mode this will add an 'assumeUnreachable' to make sure this is correct. This is needed, since the normal bind will assert if the framePushed before the bind isn't equal with the framePushed the label has.

It is my intention to have all control flow function to work with FrameAwareLabel. But this requires a lot of refactoring and would be much easier if the work on "unified" MacroAssembler is done. Also this patch is already big enough. Let's try to land this, before altering all branchXXX functions.

Currently the workaround is:
FrameAwareLabel failure;
masm.branchIf32(, failure.label());
+ failure.setFramePushed(masm.framePushed());

The last line sets the initial frame pushed if not done yet. Else it checks for a consistent state. For now I've made sure we do this once for every FrameAwareLabel, but not at every branch instruction. The refactor should fix this and it seems a bit too much to put "failure.setFramePushed(masm.framePushed());" everywhere in the code, if we will get rid of it soonish.

The patch (however big) is mostly mechanical by changing Label to FrameAwareLabel in the baseline/shared stubs.

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8676859 [details] [diff] [review]
Part10: Fix pushedFrame state

Review of attachment 8676859 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/Label.h
@@ +71,5 @@
> +        bool hadError = OOM_counter >= OOM_maxAllocations ||
> +                        (context && context->runtime->hadOutOfMemory());
> +        MOZ_ASSERT_IF(!hadError, !used());
> +    }
> +#endif

Remove this code.

@@ +88,5 @@
>    public:
>      ~Label()
>      {
> +        // TODO undo
> +        assertUsed();

Undo this change, like in the TODO comment ;)

Comment 15

[Hannes Verschore [:h4writer]]

Attachment: Part10: Fix pushedFrame state

Comment 16

[Hannes Verschore [:h4writer]]

Attachment: bug1214508-part10

more fixes (found by try push)

Comment 17

[Hannes Verschore [:h4writer]]

Comment on attachment 8677077 [details] [diff] [review]
bug1214508-part10

Review of attachment 8677077 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/x64/SharedICHelpers-x64.h
@@ +54,5 @@
>      masm.loadPtr(Address(ICStubReg, (int32_t) monitorStubOffset), ICStubReg);
>  
>      // Jump to the stubcode.
>      masm.jmp(Operand(ICStubReg, (int32_t) ICStub::offsetOfStubCode()));
> +    masm.adjustFrame(-sizeof(intptr_t));

masm.implicitPop(sizeof(intptr_t));

everywhere where I do masm.adjustFrame(-sizeof(XXXX));
The later causes implicit conversions that building x64 fails on.

Comment 18

[Jan de Mooij [:jandem]]

Are there a lot of places that need the FrameAwareLabel instead of a plain Label?

It makes it harder to read the code and the setFramePushed calls are a bit annoying, so I'd really like to avoid it.

Comment 19

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #18)
> Are there a lot of places that need the FrameAwareLabel instead of a plain
> Label?
> 
> It makes it harder to read the code and the setFramePushed calls are a bit
> annoying, so I'd really like to avoid it.

The intention is that there is no difference in using FrameAwareLabel and Label. It should just work. In the grand scheme it would be cool if we could transform all Labels into FrameAwareLabels and just s/FrameAwareLabel/Label/g afterwards.

Also it is the intention that the 'setFramePushed' calls go away. They will be called by the control flow instructinos like "branchXXX", instead of in the codegen/stub code. I will do this separate (follow-up bug), since this patch was already big enough and I don't want to make pushing this even harder.

The FrameAwareLabels are not needed for things to work. The need is mostly only 'debug purpose'. It makes it much easier to detect where we pushed/popped something wrongly. I only started to add it, since it was a mess to get everything fixed wrt push/pop. It made it much easier to detect where there were faults. I could ditch FrameAwareLabel, but it is just easier to get things into a wrong state again. Therefore I think it would be better if we keep these debug tests in...

Comment 20

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8677077 [details] [diff] [review]
bug1214508-part10

Review of attachment 8677077 [details] [diff] [review]:
-----------------------------------------------------------------

I really like the idea, but making use of a function named "setFramePushed" a common thing sounds like a terrible idea knowing that the MacroAssembler variant of the function should be manipulated with caution.
I suggest you rename the function and resubmit a patch.

Also, even if I can think of a few locations where this assertion does not hold, I think this would be a good thing to do to instrument all the Label to do this check unless we want to have a  VariableFrameDepthLabel, a general rule of thumb is that the common/safest things should have shorter names.  Developers are lazy, make them safe by their intent of being lazy ;)

::: js/src/jit/BaselineIC.cpp
@@ +279,5 @@
>          leaveStubFrame(masm);
>  
>          // If no JitCode was found, then skip just exit the IC.
> +        masm.branchPtr(Assembler::Equal, R0.scratchReg(), ImmPtr(nullptr), noCompiledCode.label());
> +        noCompiledCode.setFramePushed(masm.framePushed());

Could the FrameAwareLabel be defined with a reference to the MacroAssembler, and use this reference to "save" the masm.framePushed() each time the label() function is called?

I know that this would not work in the most generic cases, but the current idioms that we used for writing masm.code() does not make us write code in which case this would not work.  Also, this won't be necessary if all labels were instrumented.

::: js/src/jit/Label.h
@@ +103,5 @@
> +    FrameAwareLabel() {
> +        framePushed_ = UNINITIALIZED;
> +    }
> +
> +    void setFramePushed(uint32_t framePushed) {

Can you rename this function saveFramePushed, instead of setFramePushed.

This is extremelly confusing while reading the rest of the code, as it might be confused with the MacroAssembler function which should be avoided.

@@ +114,5 @@
> +        MOZ_ASSERT(!framePushedMismatch());
> +    }
> +    uint32_t framePushed() {
> +        MOZ_ASSERT(framePushed_ != UNINITIALIZED && framePushed_ != MISMATCHED);
> +        return framePushed_;

I am not sure why we would need such function but if we rely on this then this is probably that our macro assembler is wrong.

It sounds to me that this infrastructure is for checking the sanity of the code that we produce, and thus all its data should be restricted to debug only builds.

Why can't we make this function be a Debug only function?

@@ +117,5 @@
> +        MOZ_ASSERT(framePushed_ != UNINITIALIZED && framePushed_ != MISMATCHED);
> +        return framePushed_;
> +    }
> +    bool framePushedMismatch() {
> +        return framePushed_ == MISMATCHED;

This is only being accessed above, maybe inline this statement in the assertion which is above.

::: js/src/jit/arm/SharedICHelpers-arm.h
@@ +139,5 @@
>      masm.makeFrameDescriptor(reg, JitFrame_BaselineStub);
>  }
>  
>  inline void
> +EmitBaselineCallVM(MacroAssembler& masm, JitCode* target)

Should this be part of this patch?

::: js/src/jit/x64/SharedICHelpers-x64.h
@@ +54,5 @@
>      masm.loadPtr(Address(ICStubReg, (int32_t) monitorStubOffset), ICStubReg);
>  
>      // Jump to the stubcode.
>      masm.jmp(Operand(ICStubReg, (int32_t) ICStub::offsetOfStubCode()));
> +    masm.adjustFrame(-sizeof(intptr_t));

Wouldn't it be best to have this as part of masm.unreachable() function only, and call it here?

Also I am not please with the implication behind this, because this means that we would want to rely on the FrameAwareLabel frameDepth to restore the one of the MacroAssembler.  I understand where you want to go with this, but this sounds like a bad practice unless all Labels are always instrumented.

If we were to generalized this kind of instrumentation, which is followed by a non-instrumented bound Label, then we might actually rely on this framePushed to have a wild stack pointer.

@@ +143,5 @@
> +    // stub. In order to make it possible to pop it. As a result we have to
> +    // fix it here, by subtracting it. Else it would be counted twice.
> +    uint32_t framePushed = masm.framePushed() - sizeof(void*);
> +
> +    uint32_t descriptor = MakeFrameDescriptor(framePushed, JitFrame_IonStub);

Should this be part of this patch ?!

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #19)
> The FrameAwareLabels are not needed for things to work. The need is mostly
> only 'debug purpose'. It makes it much easier to detect where we
> pushed/popped something wrongly. I only started to add it, since it was a
> mess to get everything fixed wrt push/pop. It made it much easier to detect
> where there were faults. I could ditch FrameAwareLabel, but it is just
> easier to get things into a wrong state again. Therefore I think it would be
> better if we keep these debug tests in...

Maybe file a separate bug for this? It's a large change and this bug already has some large patches, and we probably don't want to block this bug on it. Also if it's a separate bug more people may notice/comment.

Comment 22

[Hannes Verschore [:h4writer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edc16d9db5ed

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db0f569d52a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd698a31dbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/54b59d69c085
https://hg.mozilla.org/integration/mozilla-inbound/rev/add8a32eb849
https://hg.mozilla.org/integration/mozilla-inbound/rev/8413cd50ac68
https://hg.mozilla.org/integration/mozilla-inbound/rev/26c7fea3fb16
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ff5ed6cb4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e207a6d30e

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6139e8bad12

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2ece1c131b

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db0f569d52a3
https://hg.mozilla.org/mozilla-central/rev/fcd698a31dbe
https://hg.mozilla.org/mozilla-central/rev/54b59d69c085
https://hg.mozilla.org/mozilla-central/rev/add8a32eb849
https://hg.mozilla.org/mozilla-central/rev/8413cd50ac68
https://hg.mozilla.org/mozilla-central/rev/26c7fea3fb16
https://hg.mozilla.org/mozilla-central/rev/b7ff5ed6cb4c
https://hg.mozilla.org/mozilla-central/rev/54e207a6d30e
https://hg.mozilla.org/mozilla-central/rev/c6139e8bad12
https://hg.mozilla.org/mozilla-central/rev/bb2ece1c131b