Closed Bug 1214508 Opened 5 years ago Closed 5 years ago

[[SharedStubs] Port GetProp

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(10 files, 2 obsolete files)

221.58 KB, patch
jandem
: review+
Details | Diff | Splinter Review
45.66 KB, patch
jandem
: review+
Details | Diff | Splinter Review
56.90 KB, patch
jandem
: review+
Details | Diff | Splinter Review
13.15 KB, patch
jandem
: review+
Details | Diff | Splinter Review
31.34 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.58 KB, patch
jandem
: review+
Details | Diff | Splinter Review
49.38 KB, patch
jandem
: review+
Details | Diff | Splinter Review
24.31 KB, patch
jandem
: review+
Details | Diff | Splinter Review
4.55 KB, patch
jandem
: review+
Details | Diff | Splinter Review
283.12 KB, patch
Details | Diff | Splinter Review
No description provided.
Blocks: 1161516
Assignee: nobody → hv1989
Attachment #8674106 - Flags: review?(jdemooij)
Attachment #8674112 - Flags: review?(jdemooij)
Attachment #8674116 - Flags: review?(jdemooij)
Attachment #8674126 - Flags: review?(jdemooij)
Attachment #8674106 - Flags: review?(jdemooij) → review+
Attachment #8674107 - Flags: review?(jdemooij) → review+
So part 1 doesn't build without part 2? It's nice to merge them in that case to not break bisecting etc.
Attachment #8674111 - Flags: review?(jdemooij) → review+
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.
Attachment #8674112 - Flags: review?(jdemooij) → review+
Attachment #8674116 - Flags: review?(jdemooij) → review+
Attachment #8674118 - Flags: review?(jdemooij) → review+
Attachment #8674125 - Flags: review?(jdemooij) → review+
Attachment #8674126 - Flags: review?(jdemooij) → review+
Attachment #8674114 - Flags: review?(jdemooij) → review+
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 :)
Attached patch Part10: Fix pushedFrame state (obsolete) — Splinter Review
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.
Attachment #8676859 - Flags: review?(jdemooij)
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 ;)
Attached patch Part10: Fix pushedFrame state (obsolete) — Splinter Review
Attachment #8676859 - Attachment is obsolete: true
Attachment #8676859 - Flags: review?(jdemooij)
Attachment #8677037 - Flags: review?(jdemooij)
more fixes (found by try push)
Attachment #8677037 - Attachment is obsolete: true
Attachment #8677037 - Flags: review?(jdemooij)
Attachment #8677077 - Flags: review?(jdemooij)
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.
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.
Flags: needinfo?(hv1989)
(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...
Flags: needinfo?(hv1989)
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 ?!
(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.
Attachment #8677077 - Flags: review?(jdemooij)
Depends on: 1226188
Depends on: 1249252
You need to log in before you can comment on or make changes to this bug.