Closed Bug 1088316 Opened 10 years ago Closed 9 years ago

Implement callee-side return address pushing

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mjrosenb, Assigned: mjrosenb)

References

Details

Attachments

(1 file)

Currently, ARM32 generates the annoying sequence:
push pc;
blx address
for calls, so we can pretend than we're x86, and push the return address in the caller side. The ARM abi for the most part doesn't do this (not that we're following the abi anywhere else). For the transition to ARMv8/arm64/aarch64, there isn't a good way to push the return address before making a function call, so we'll need to trasinion to a callee-pushed abi for callee-pushed architectures.  The current plan is to implement this on arm32, then transition that code to arm64.
\o/
lmk if you need help in GenerateFFIIonExit.
Blocks: 1088326
Attached patch push_lr-r1.patchSplinter Review
This passes jit tests on arm32, I don't think I've changed anything on x86 or x64.  Should probably do a try run for android/b2g coverage, but I'm pretty optimistic about it.
Attachment #8518679 - Flags: review?(jdemooij)
Comment on attachment 8518679 [details] [diff] [review]
push_lr-r1.patch

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

Looks good, r=me with comments below addressed.

::: js/src/jit/BaselineIC.cpp
@@ +653,4 @@
>      JitCode *code = cx->runtime()->jitRuntime()->getVMWrapper(fun);
>      if (!code)
>          return false;
> +    MOZ_ASSERT(fun.expectTailCall == TailCall);

Nit: newline before this line.

@@ +665,4 @@
>      JitCode *code = cx->runtime()->jitRuntime()->getVMWrapper(fun);
>      if (!code)
>          return false;
> +    MOZ_ASSERT(fun.expectTailCall == NonTailCall);

And here. Also, please add the following to your .hgrc or somewhere else so that patches have more context:

[diff]
unified = 8

::: js/src/jit/CodeGenerator.cpp
@@ +1072,5 @@
>          return false;
> +#ifdef JS_USE_LINK_REGISTER
> +    if (mode != RegExpShared::MatchOnly) {
> +        masm.pushReturnAddress();
> +    }

Nit: no {}

@@ +5988,5 @@
>      Register forkJoinContext = CallTempReg4;
>  
>      Label failure, failurePopTemps;
> +#ifdef JS_USE_LINK_REGISTER
> +    if (!getenv("OLD_CALL"))

Remove debugging code.

@@ +6103,5 @@
>      MacroAssembler masm(cx);
>  
>      RegisterSet regs = RegisterSet::Volatile();
> +#ifdef JS_USE_LINK_REGISTER
> +    if (!getenv("OLD_CALL"))

And here.

@@ +6149,5 @@
>      const Register regSlots = CallTempReg0;
>  
>      MacroAssembler masm(cx);
> +#ifdef JS_USE_LINK_REGISTER
> +    if (!getenv("OLD_CALL"))

And here and a bunch of times below.

@@ +6204,5 @@
>      masm.callWithExitFrame(&call);
> +#ifdef JS_USE_LINK_REGISTER
> +    // sigh, this should probably attempt to bypass the push lr that starts off the block
> +    // but oh well.
> +    if (!getenv("OLD_CALL")) {

I'd remove the comment or make it more neutral. Since a few extra instructions here is not perf-critical, I'd say just remove it.

::: js/src/jit/VMFunctions.h
@@ +40,4 @@
>      { }
>  };
>  
> +enum TC {

Nit: prefer a clearer name, "MaybeTailCall" or something.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3713,5 @@
>      // Since we just write the return address into the stack, which is popped on
>      // return, the net effect is removing 4 bytes from the stack.
> +    AutoForbidPools afp(this, 3);
> +    // TODO: file follow-up bug about never using this function,
> +    // since it is actually useless.

Please file the bug and mention the bug number here.

@@ +3719,4 @@
>      as_blx(r);
>  }
>  
> +static int x;

Remove.

@@ +3738,5 @@
>      // is restored to its unaligned state.
>      AutoForbidPools afp(this, 2);
> +    if (getenv("OLD_CALL")) {
> +        ma_push(pc);
> +    }

Remove.

::: js/src/jit/arm/Trampoline-arm.cpp
@@ +751,5 @@
>      // We're aligned to an exit frame, so link it up.
> +    // If it isn't a tail call, then the return address needs to be saved
> +    if (f.expectTailCall == NonTailCall) {
> +        masm.pushReturnAddress();
> +    }

Nit: no {}

@@ +933,5 @@
>      masm.callWithABI(IonMarkFunction(type));
> +    if (!getenv("OLD_CALL")) {
> +        save.take(AnyRegister(lr));
> +        save.add(pc);
> +    }

Remove debugging code.

@@ +938,3 @@
>      masm.PopRegsInMask(save);
> +    if (getenv("OLD_CALL"))
> +        masm.ret();

And here.

::: js/src/jit/shared/Assembler-shared.h
@@ +17,4 @@
>  #include "jit/Registers.h"
>  #include "jit/RegisterSets.h"
>  #include "vm/HelperThreads.h"
> +#if defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_ARM64)

Nit: add a blank line before and after this #if/#endif block.
Attachment #8518679 - Flags: review?(jdemooij) → review+
Can this patch be landed?
Flags: needinfo?(mrosenberg)
https://hg.mozilla.org/mozilla-central/rev/4ca1865c2102
Assignee: nobody → mrosenberg
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.