[webkit-reviews] review denied: [Bug 40179] Re-enable JIT_OPTIMIZE_NATIVE_CALL on MIPS : [Attachment 57924] Returned value in a register
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 21 17:48:27 PDT 2010
Oliver Hunt <oliver at apple.com> has denied Chao-ying Fu <fu at mips.com>'s request
for review:
Bug 40179: Re-enable JIT_OPTIMIZE_NATIVE_CALL on MIPS
https://bugs.webkit.org/show_bug.cgi?id=40179
Attachment 57924: Returned value in a register
https://bugs.webkit.org/attachment.cgi?id=57924&action=review
------- Additional Comments from Oliver Hunt <oliver at apple.com>
Sorry for the delay in review
> +#if CPU(MIPS)
> + // move the returned address to regT1
> + move(MIPSRegisters::ra, regT1);
> +#else
> peek(regT1);
> +#endif
This is bad, we should simply have a loadReturnAddress(RegisterID dst) function
to do this, rather than having an architecture check everywhere we need to do
this (eg. ARM, PPC, etc would need to have a branch as well)
> emitPutToCallFrameHeader(regT1, RegisterFile::ReturnPC);
>
> #if CPU(X86_64)
> @@ -204,6 +209,31 @@ JIT::Label JIT::privateCompileCTINativeC
>
> addPtr(Imm32(16 - sizeof(void*)), stackPointerRegister);
>
> +#elif CPU(MIPS)
> + // Calling convention: f(a0, a1, a2, a3);
> + // Host function signature: f(ExecState*);
> +
> + // Allocate stack space for 24 bytes (8-byte aligned)
> + // 16 bytes (unused) for 4 arguments
> + // 4 bytes (unused)
> + // 4 bytes for the returned address
> + subPtr(Imm32(24), stackPointerRegister);
> +
> + // Save the returned address to 20($sp)
> + storePtr(MIPSRegisters::ra, Address(stackPointerRegister, 20));
> +
> + // Setup arg0
> + move(callFrameRegister, MIPSRegisters::a0);
> +
> + // Call
> + emitGetFromCallFrameHeaderPtr(RegisterFile::Callee, MIPSRegisters::a2);
> + loadPtr(Address(MIPSRegisters::a2, OBJECT_OFFSETOF(JSFunction,
m_executable)), regT2);
> + move(regT0, callFrameRegister); // Eagerly restore caller frame register
to avoid loading from stack.
> + call(Address(regT2, executableOffsetToFunction));
> +
> + // Restore stack space
> + addPtr(Imm32(24), stackPointerRegister);
> +
Our normal model for a callframe this complicated is to make a struct that
describes it and then use OBJECT_OFFSETOF to do the stores
> +#if CPU(MIPS)
> + loadPtr(Address(stackPointerRegister, -4), MIPSRegisters::ra);
> +#endif
-4? should be -sizeof(void*)?
> +
> // Return.
> ret();
>
> // Handle an exception
> exceptionHandler.link(this);
> // Grab the return address.
> +#if CPU(MIPS)
> + loadPtr(Address(stackPointerRegister, -4), regT1);
> +#else
> peek(regT1);
> +#endif
why doesn't peek() work here?
> +#if CPU(MIPS)
> + poke(callFrameRegister, OBJECT_OFFSETOF(struct JITStackFrame, callFrame)
/ sizeof(void*));
> + move(ImmPtr(FunctionPtr(ctiVMThrowTrampoline).value()),
MIPSRegisters::ra);
> +#else
> poke(callFrameRegister, 1 + OBJECT_OFFSETOF(struct JITStackFrame,
callFrame) / sizeof (void*));
> poke(ImmPtr(FunctionPtr(ctiVMThrowTrampoline).value()));
> +#endif
a setReturnAddress function is probably better here
More information about the webkit-reviews
mailing list