Move functions which are dealing with the Jit frames, such as makeFrameDescriptor, buildFakeExitFrame, callWithExitFrame, … to the generic MacroAssembler.

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.

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?

(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.

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).

(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.

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.

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.

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.

(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?

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.

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?

(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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0de036b80c64 https://hg.mozilla.org/integration/mozilla-inbound/rev/c35d8212f835 https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d6baa60ff9 https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b5bd4c0dc4 https://hg.mozilla.org/integration/mozilla-inbound/rev/fd58eaa164a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/4217b8a08683 https://hg.mozilla.org/integration/mozilla-inbound/rev/54b64ecadcdb https://hg.mozilla.org/integration/mozilla-inbound/rev/bd50f52c3bbe

https://hg.mozilla.org/mozilla-central/rev/0de036b80c64 https://hg.mozilla.org/mozilla-central/rev/c35d8212f835 https://hg.mozilla.org/mozilla-central/rev/a3d6baa60ff9 https://hg.mozilla.org/mozilla-central/rev/e7b5bd4c0dc4 https://hg.mozilla.org/mozilla-central/rev/fd58eaa164a7 https://hg.mozilla.org/mozilla-central/rev/4217b8a08683 https://hg.mozilla.org/mozilla-central/rev/54b64ecadcdb https://hg.mozilla.org/mozilla-central/rev/bd50f52c3bbe