Move MacroAssembler Jit calls functions to the generic macro assembler.

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

20.34 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
13.32 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
23.76 KB, patch
jandem
: review+
Details | Diff | Splinter Review
14.53 KB, patch
djvj
: review+
Details | Diff | Splinter Review
20.12 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
27.00 KB, patch
jandem
: review+
Details | Diff | Splinter Review
18.76 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
11.22 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Move functions which are dealing with the Jit frames, such as makeFrameDescriptor, buildFakeExitFrame, callWithExitFrame, … to the generic MacroAssembler.
(Assignee)

Updated

3 years ago
Depends on: 1103108
(Assignee)

Comment 1

3 years ago
Created attachment 8647565 [details] [diff] [review]
part 1 - Factor MacroAssembler::callJit.
Attachment #8647565 - Flags: review?(benj)
(Assignee)

Comment 2

3 years ago
Created attachment 8647569 [details] [diff] [review]
part 2 - Factor MacroAssembler::makeFrameDescriptor.
Attachment #8647569 - Flags: review?(sstangl)
(Assignee)

Comment 3

3 years ago
Created attachment 8647570 [details] [diff] [review]
part 3 - Use RAII for redundant profilerPreCall and profilerPostReturn calls.
Attachment #8647570 - Flags: review?(kvijayan)
(Assignee)

Comment 4

3 years ago
Created attachment 8647571 [details] [diff] [review]
part 4 - Factor MacroAssembler::callWithExitFrame.
Attachment #8647571 - Flags: review?(jdemooij)
Comment on attachment 8647569 [details] [diff] [review]
part 2 - Factor MacroAssembler::makeFrameDescriptor.

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

::: js/src/jit/MacroAssembler-inl.h
@@ +169,5 @@
> +{
> +    // See JitFrames.h for a description of the frame descriptor format.
> +
> +    lshiftPtr(Imm32(FRAMESIZE_SHIFT), frameSizeReg);
> +    // The saved-frame bit is zero for new frames. See js::SavedStacks

ultra nit: missing final period.

::: js/src/jit/MacroAssembler.h
@@ +571,5 @@
>      inline uint32_t callJit(Register callee);
>  
> +    // The frame descriptor is the second field of all Jit frames, pushed before
> +    // calling the Jit function.  It is a composite value which is defined in
> +    // JitFrames.h

ultra nit: missing final period. Can also put "JitFrames.h" into the previous line by removing "which is".

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ -2637,5 @@
>      }
>  
>      void handleFailureWithHandlerTail(void* handler);
>  
> -    // FIXME: This is the same on all platforms. Can be common code?

Indeed.
Attachment #8647569 - Flags: review?(sstangl) → review+
Comment on attachment 8647565 [details] [diff] [review]
part 1 - Factor MacroAssembler::callJit.

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

Alright. Can you also delete callJitHalfPush, as you've replaced the call() implementation code with callJitHalfPush's code?
There seems to be a few cases where we'll do more work than we actually ought to, but I guess you've thought about that already. Do you have any measurements, by luck? (in particular for ARM AsmJS calls)

::: js/src/jit/MacroAssembler.h
@@ +556,5 @@
> +    // ===============================================================
> +    // Jit Frames.
> +    //
> +    // These functions are used to build the content of the Jit frames.  See
> +    // CommonFrameLayout class, and all its derivative. The content should be

nit: derivatives

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +5071,5 @@
> +
> +uint32_t
> +MacroAssembler::callJitNoProfiler(Register callee)
> +{
> +    // The return address is pushed by callee, which push the link register

nit: pushes

::: js/src/jit/arm/Trampoline-arm.cpp
@@ +347,5 @@
>      // the stack would be aligned once the call is complete.
>      masm.assertStackAlignment(JitStackAlignment, sizeof(uintptr_t));
>  
>      // Call the function.
> +    masm.callJitNoProfiler(r0);

as the return value isn't even used, why not just use masm.call(reg_code) here? (ditto for other platforms) Do you plan to share the generateEnterJIT code?

@@ -347,5 @@
>      // the stack would be aligned once the call is complete.
>      masm.assertStackAlignment(JitStackAlignment, sizeof(uintptr_t));
>  
>      // Call the function.
> -    masm.ma_callJitHalfPush(r0);

Couldn't callJitHalfPush be removed entirely, as it does the same thing as callJitNoProfiler?

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ -2685,5 @@
>          MOZ_CRASH("callExit");
>      }
>  
> -    void callJitFromAsmJS(Register reg) {
> -        Blr(ARMRegister(reg, 64));

So we'll sync the stackptr why we don't even need to, in the case of AsmJS?

::: js/src/jit/arm64/Trampoline-arm64.cpp
@@ +224,5 @@
>      }
>  
>      // Call function.
>      // Since AArch64 doesn't have the pc register available, the callee must push lr.
> +    masm.callJitNoProfiler(reg_code);

(see comment in arm's trampoline)

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +3620,5 @@
> +
> +uint32_t
> +MacroAssembler::callJitNoProfiler(Register callee)
> +{
> +    // This is a MIPS hack to push return address during jalr delay slot.

So this is copy/pasto from callJitHalfPush. Does it mean, as for ARM, that we'll be doing more work here than in cases where we shouldn't need the hack?

::: js/src/jit/mips32/Trampoline-mips32.cpp
@@ +302,5 @@
>      // the stack would be aligned once the call is complete.
>      masm.assertStackAlignment(JitStackAlignment, sizeof(uintptr_t));
>  
>      // Call the function with pushing return address to stack.
> +    masm.callJitNoProfiler(reg_code);

(see comment in arm's trampoline)

::: js/src/jit/none/MacroAssembler-none.h
@@ -200,5 @@
>      void callWithExitFrame(Label*) { MOZ_CRASH(); }
>      void callWithExitFrame(JitCode*) { MOZ_CRASH(); }
>      void callWithExitFrame(JitCode*, Register) { MOZ_CRASH(); }
>  
> -    void callJit(Register callee) { MOZ_CRASH(); }

So you can delete this one because the PER_SHARED_ARCH annotation in MacroAssembler.h implements the equivalent crashing function, right?
Attachment #8647565 - Flags: review?(benj) → review+
(Assignee)

Comment 7

3 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Comment on attachment 8647565 [details] [diff] [review]
> part 1 - Factor MacroAssembler::callJit.
> 
> Review of attachment 8647565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Alright. Can you also delete callJitHalfPush, as you've replaced the call()
> implementation code with callJitHalfPush's code?

Indeed, I had planned to do it in a follow-up patch, as this function was still used by callWithExitFrame.

> There seems to be a few cases where we'll do more work than we actually
> ought to, but I guess you've thought about that already. Do you have any
> measurements, by luck? (in particular for ARM AsmJS calls)

I don't have measurements, as the only measurements I can make are running within the simulator which are likely way more louder than whatever impact this modification could have.

My hope is to get rid of the syncStack/setupUnalignedABICall calls, as soon as we can ensure that we can trust the framePushed() value, which is not the case at the moment.

> ::: js/src/jit/arm/Trampoline-arm.cpp
> @@ +347,5 @@
> >      // the stack would be aligned once the call is complete.
> >      masm.assertStackAlignment(JitStackAlignment, sizeof(uintptr_t));
> >  
> >      // Call the function.
> > +    masm.callJitNoProfiler(r0);
> 
> as the return value isn't even used, why not just use masm.call(reg_code)
> here? (ditto for other platforms) Do you plan to share the generateEnterJIT
> code?

I do not use masm.call here, because I want to highlight that we are calling with a Jit frame on the stack.
Also, I want to reduce the differences between the trampoline.  I wish we could share the trampoline code, but we are not yet there.

> ::: js/src/jit/arm64/MacroAssembler-arm64.h
> @@ -2685,5 @@
> >          MOZ_CRASH("callExit");
> >      }
> >  
> > -    void callJitFromAsmJS(Register reg) {
> > -        Blr(ARMRegister(reg, 64));
> 
> So we'll sync the stackptr why we don't even need to, in the case of AsmJS?

I would not bet on the fact that it was not needed, especially since all calls have a syncStack call, except this one.  And this is the only platform which has such a difference, so I am going for the safest option, and hope we would be able to optimize this later.

> ::: js/src/jit/mips32/MacroAssembler-mips32.cpp
> @@ +3620,5 @@
> > +
> > +uint32_t
> > +MacroAssembler::callJitNoProfiler(Register callee)
> > +{
> > +    // This is a MIPS hack to push return address during jalr delay slot.
> 
> So this is copy/pasto from callJitHalfPush. Does it mean, as for ARM, that
> we'll be doing more work here than in cases where we shouldn't need the hack?

No, this "hack" is just a way to use the delay-slot to push the return address, and thus prevents us from having to rely on a link-register, as on ARM / ARM64.  If we were not using the delay-slot, we would have to have an extra instruction which would be executed any way.

So, this hack, which use the delay-slot is an optimization.

> ::: js/src/jit/none/MacroAssembler-none.h
> @@ -200,5 @@
> >      void callWithExitFrame(Label*) { MOZ_CRASH(); }
> >      void callWithExitFrame(JitCode*) { MOZ_CRASH(); }
> >      void callWithExitFrame(JitCode*, Register) { MOZ_CRASH(); }
> >  
> > -    void callJit(Register callee) { MOZ_CRASH(); }
> 
> So you can delete this one because the PER_SHARED_ARCH annotation in
> MacroAssembler.h implements the equivalent crashing function, right?

We can remove this one, because it is now fully inlined in MacroAssembler-inl.h.
We do not have to add callJitNoProfiler here, because the PER_SHARED_ARCH macro add the { MOZ_CRASH(); } in the MacroAssembler.h file.
Comment on attachment 8647571 [details] [diff] [review]
part 4 - Factor MacroAssembler::callWithExitFrame.

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

This is so much nicer.

::: js/src/jit/MacroAssembler-inl.h
@@ +180,5 @@
>      orPtr(Imm32(type), frameSizeReg);
>  }
>  
> +uint32_t
> +MacroAssembler::callWithExitFrame(JitCode* callee, const Register* dynStack)

Is CodeGeneratorShared::callVM the only caller of this function? If yes, it might make sense to simply inline the code there. Either way is fine with me though.
Attachment #8647571 - Flags: review?(jdemooij) → review+
Comment on attachment 8647570 [details] [diff] [review]
part 3 - Use RAII for redundant profilerPreCall and profilerPostReturn calls.

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

::: js/src/jit/MacroAssembler.h
@@ +1217,5 @@
>    private:
> +    // This class is used to surround call sites throughout the assembler. This
> +    // is used by callWithABI, callJit, and callWithExitFrame functions, except
> +    // if suffixed by NoProfiler.
> +    class CallProfiler {

This name sounds like you're calling the profiler, which is misleading.

|AutoProfilerCallInstrumentation| would be more descriptive.

@@ +1222,5 @@
> +        MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER;
> +
> +      public:
> +        explicit CallProfiler(MacroAssembler& masm MOZ_GUARD_OBJECT_NOTIFIER_PARAM);
> +        ~CallProfiler() {}

Destructor of CallProfiler doesn't do anything, why?

THe places where you are replacing the explicit method calls with this RAII helper, you are removing a |profilerPostReturn|.  But the destructor doesn't do anything, so where does the logic in |profilerPostReturn| get executed?

@@ -1550,5 @@
>      }
> -
> -    void profilerPreCallImpl();
> -    void profilerPreCallImpl(Register reg, Register reg2);
> -    void profilerPostReturnImpl() {}

It would be good if you could keep these Impl methods around, and have |CallProfiler| call them instead of duplicating their internals within the constructor/destructor of CallProfiler?

I can foresee situations where we want to do the pre-call instrumentation, but not post-return instrumentation (e.g. for Baseline IC style "no-return" calls).
Attachment #8647570 - Flags: review?(kvijayan)
(Assignee)

Comment 10

3 years ago
(In reply to Kannan Vijayan [:djvj] from comment #9)
> Comment on attachment 8647570 [details] [diff] [review]
> part 3 - Use RAII for redundant profilerPreCall and profilerPostReturn calls.
> 
> Review of attachment 8647570 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1222,5 @@
> > +        MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER;
> > +
> > +      public:
> > +        explicit CallProfiler(MacroAssembler& masm MOZ_GUARD_OBJECT_NOTIFIER_PARAM);
> > +        ~CallProfiler() {}
> 
> Destructor of CallProfiler doesn't do anything, why?

Because all the code that we had was checking if we had to do nothing in all cases.

> @@ -1550,5 @@
> >      }
> > -
> > -    void profilerPreCallImpl();
> > -    void profilerPreCallImpl(Register reg, Register reg2);
> > -    void profilerPostReturnImpl() {}
> 
> It would be good if you could keep these Impl methods around, and have
> |CallProfiler| call them instead of duplicating their internals within the
> constructor/destructor of CallProfiler?
> 
> I can foresee situations where we want to do the pre-call instrumentation,
> but not post-return instrumentation (e.g. for Baseline IC style "no-return"
> calls).

For the moment, we have no such instrumentation, I think we should add these functions back if we really care about it.  In the mean time we should just remove them.
(Assignee)

Comment 11

3 years ago
Created attachment 8652362 [details] [diff] [review]
part 3 - Use RAII for redundant profilerPreCall and profilerPostReturn calls.
Attachment #8647570 - Attachment is obsolete: true
Attachment #8652362 - Flags: review?(kvijayan)
(Assignee)

Comment 12

3 years ago
Created attachment 8652364 [details] [diff] [review]
part 5 - Factor MacroAssembler::buildFakeExitFrame.
Attachment #8652364 - Flags: review?(hv1989)
(Assignee)

Comment 13

3 years ago
Created attachment 8652365 [details] [diff] [review]
part 6 - Move MacroAssembler ExitFrameFooter function in the check_macroassembler_style section.

Side-note: I noticed that enterFakeExitFrame was only used with ExitFrame
tokens.  Thus I removed some of the cast on which we relied on, and used the
enumerated values instead.
Attachment #8652365 - Flags: review?(jdemooij)
(Assignee)

Comment 14

3 years ago
Created attachment 8652366 [details] [diff] [review]
part 7 - Factor MacroAssembler::callAndPushreturnAddress on architecture where this is efficient.
Attachment #8652366 - Flags: review?(sstangl)
(Assignee)

Comment 15

3 years ago
Created attachment 8652367 [details] [diff] [review]
part 8 - Remove MacroAssemblerSpecific::ma_callJitHalfPush.
Attachment #8652367 - Flags: review?(benj)
(Assignee)

Comment 16

3 years ago
Comment on attachment 8652365 [details] [diff] [review]
part 6 - Move MacroAssembler ExitFrameFooter function in the check_macroassembler_style section.

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

::: js/src/jit/MacroAssembler.cpp
@@ +2871,5 @@
> +void
> +MacroAssembler::linkSelfReference(JitCode* code)
> +{
> +    // If this code can transition to C++ code and witness a GC, then we need to store
> +    // the JitCode onto the stack in order to GC it correctly.  exitCodePatch should

self-nit: exitCodePatch should be renamed to selfReferencePatch_
Comment on attachment 8652367 [details] [diff] [review]
part 8 - Remove MacroAssemblerSpecific::ma_callJitHalfPush.

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

Code removal \o/

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +524,5 @@
>      // Note that this code assumes the function is JITted.
>      masm.andq(Imm32(uint32_t(CalleeTokenMask)), rax);
>      masm.loadPtr(Address(rax, JSFunction::offsetOfNativeOrScript()), rax);
>      masm.loadBaselineOrIonRaw(rax, rax, nullptr);
> +    uint32_t returnOffset = masm.callJitNoProfiler(rax);

It's ok to land this as part of this patch, but it really should belong to another patch of this bug.
Attachment #8652367 - Flags: review?(benj) → review+

Updated

3 years ago
Attachment #8652362 - Flags: review?(kvijayan) → review+
Comment on attachment 8652366 [details] [diff] [review]
part 7 - Factor MacroAssembler::callAndPushreturnAddress on architecture where this is efficient.

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

::: js/src/jit/BaselineCompiler.cpp
@@ +3770,5 @@
>      // Push a fake return address on the stack. We will resume here when the
>      // generator returns.
>      Label genStart, returnTarget;
> +#ifdef JS_USE_LINK_REGISTER
> +    masm.call(&genStart);

This really shouldn't even call anything... On ARM64, it could just do masm.Adr(scratch, &genStart), which puts the address into some scratch register. ARM and MIPS should have something similar.

@@ +3782,5 @@
>  
>      masm.jump(&returnTarget);
>      masm.bind(&genStart);
> +#ifdef JS_USE_LINK_REGISTER
> +    masm.pushReturnAddress();

Then maybe this line of code could be removed, with just a push of the scratch register above.
Attachment #8652366 - Flags: review?(sstangl) → review+
(Assignee)

Comment 19

3 years ago
(In reply to Sean Stangl [:sstangl] from comment #18)
> part 7 - Factor MacroAssembler::callAndPushreturnAddress on architecture
> where this is efficient.
> 
> ::: js/src/jit/BaselineCompiler.cpp
> @@ +3770,5 @@
> >      // Push a fake return address on the stack. We will resume here when the
> >      // generator returns.
> >      Label genStart, returnTarget;
> > +#ifdef JS_USE_LINK_REGISTER
> > +    masm.call(&genStart);
> 
> This really shouldn't even call anything... On ARM64, it could just do
> masm.Adr(scratch, &genStart), which puts the address into some scratch
> register. ARM and MIPS should have something similar.

MIPS does not use a link register, the delay slot is used for pushing the return address on the stack.

On ARM, we can use the fact that the pushed pc is +8 above the current one, to do something like:

  ld scratch, …
  push pc
  bx scratch

But I do not understand what you want to do for ARM64, while reading the comment I thought you wanted to get rid of this function?   Do you have a better idea?
Flags: needinfo?(sstangl)
Comment on attachment 8652365 [details] [diff] [review]
part 6 - Move MacroAssembler ExitFrameFooter function in the check_macroassembler_style section.

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

::: js/src/jit/MacroAssembler-inl.h
@@ +203,5 @@
>  
>  void
>  MacroAssembler::PushStubCode()
>  {
> +    // Make sure that we do not erase an existing self-refence.

Nit: reference (typo)

::: js/src/jit/MacroAssembler.cpp
@@ +2355,5 @@
>  void
>  MacroAssembler::link(JitCode* code)
>  {
>      MOZ_ASSERT(!oom());
> +    linkSelfReference(code);

Hm this is the only caller so it seems simpler to inline the code here.

::: js/src/jit/MacroAssembler.h
@@ +595,5 @@
> +  public:
> +    // ===============================================================
> +    // Exit frame footer.
> +    //
> +    // When calling out-side the Jit we push an exit frame. To mark the stack

Nit: s/out-side/outside/

@@ +604,5 @@
> +
> +    // If the current piece of code might be garbage collected, then the exit
> +    // frame footer must contain a pointer to the current JitCode, such that the
> +    // garbage collector can keep the code alive as long this code is on the
> +    // stack. This function push a placeholder which is replaced when the code

Nit: s/push/pushes/

@@ +608,5 @@
> +    // stack. This function push a placeholder which is replaced when the code
> +    // is linked.
> +    inline void PushStubCode();
> +
> +    // Return true, if the code contains a self-reference which needs to be

Nit: remove the ','

@@ +612,5 @@
> +    // Return true, if the code contains a self-reference which needs to be
> +    // patched when the code is linked.
> +    inline bool hasSelfReference() const;
> +
> +    // Push stub code, and the VMFunction pointer.

Nit: remove the ','

@@ +617,5 @@
> +    inline void enterExitFrame(const VMFunction* f = nullptr);
> +
> +    // Push an exit frame token, to identify which fake exit frame this footer
> +    // corresponds to.
> +    inline void enterFakeExitFrame(enum ExitFrameTokenValues token);

Pre-existing and not for this patch, but the plural Values is a bit unusual compared to other enums (like MIRType, JSValueType etc). I think it'd be nice to have

enum class ExitFrameToken {
    Native = 0x0,
    IonDOMGetter = 0x1,
    ...
    Bare = 0xFF
};

And then use ExitFrameToken::Native etc. But that's unrelated to this patch.
Attachment #8652365 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

3 years ago
Blocks: 1199710
Comment on attachment 8652364 [details] [diff] [review]
part 5 - Factor MacroAssembler::buildFakeExitFrame.

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

::: js/src/jit/MacroAssembler-inl.h
@@ +189,5 @@
> +
> +uint32_t
> +MacroAssembler::buildFakeExitFrame(Register scratch)
> +{
> +    mozilla::DebugOnly<uint32_t> initialDepth = framePushed();

Style nit: can you add an newline after this line?
Attachment #8652364 - Flags: review?(hv1989) → review+
(Assignee)

Updated

3 years ago
Flags: needinfo?(sstangl)
(Assignee)

Comment 22

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #20)
> Comment on attachment 8652365 [details] [diff] [review]
> part 6 - Move MacroAssembler ExitFrameFooter function in the
> check_macroassembler_style section.
> 
> ::: js/src/jit/MacroAssembler.cpp
> @@ +2355,5 @@
> >  void
> >  MacroAssembler::link(JitCode* code)
> >  {
> >      MOZ_ASSERT(!oom());
> > +    linkSelfReference(code);
> 
> Hm this is the only caller so it seems simpler to inline the code here.

I created this function such that MacroAssembler::link can be a list of independent function calls, and that linkSelfReference can be moved inside the only section which deals with selfReference.

I think this is better to keep this function separated to keep the code structured in the long term, and avoid bloating the MacroAssembler::link function with various unrelated pieces.


> > +    inline void enterFakeExitFrame(enum ExitFrameTokenValues token);
> 
> Pre-existing and not for this patch, but the plural Values is a bit unusual
> compared to other enums (like MIRType, JSValueType etc). I think it'd be
> nice to have
> 
> enum class ExitFrameToken {
>     Native = 0x0,
>     IonDOMGetter = 0x1,
>     ...
>     Bare = 0xFF
> };
> 
> And then use ExitFrameToken::Native etc. But that's unrelated to this patch.

I opened Bug 1199710 to address this issue.
I think this would make a good first bug, as soon as these patches land.
You need to log in before you can comment on or make changes to this bug.