Bug 146946 – jsc-tailcall: LLint, Baseline and DFG JIT should save & restore platform's callee-save registers

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

Save and restore platform callee save registers in the LLInt and baseline JIT.  We only do this for 64 bit builds as it isn't needed nor expected to be profitable for 32 bit builds.

[Michael Saboff]

Created attachment 256803 [details]
Patch

[Basile Clement]

Comment on attachment 256803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256803&action=review

Looks good to me overall. Does this correctly restores callee-save registers on throw? We should probably test it with DFG disabled and without preserveCalleeSave() in vmEntryToJavascript.

> Source/JavaScriptCore/ChangeLog:10
> +        This space is maximum space needed for the callee registers that the LLInt  or baseline JIT

is maximum space -> is the maximum space

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3295
> +#endif

Isn't this the same as sizeof(Register)?

> Source/JavaScriptCore/jit/AssemblyHelpers.h:187
> +                storePtr(reg.gpr(), Address(framePointerRegister, -(sizeof(void*) * registerIndex--)));

Shouldn't sizeof(void*) be sizeof(Register) instead?

> Source/JavaScriptCore/jit/AssemblyHelpers.h:201
> +                loadPtr(Address(framePointerRegister, -(sizeof(void*) * registerIndex--)), reg.gpr());

Ditto.

> Source/JavaScriptCore/jit/GPRInfo.h:329
> +    static const int numberLLIntBaselineCalleeSaveRegisters = 0;

I think numberOfLLIntBaselineCalleeSaveRegisters is nicer.

> Source/JavaScriptCore/jit/RegisterSet.cpp:150
> +    UNREACHABLE_FOR_PLATFORM();

What about MIPS and SH4?

[Michael Saboff]

(In reply to comment #2)
> Comment on attachment 256803 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256803&action=review
> 
> Looks good to me overall. Does this correctly restores callee-save registers
> on throw? We should probably test it with DFG disabled and without
> preserveCalleeSave() in vmEntryToJavascript.

No, that will be done in an unwinder patch.
 
> > Source/JavaScriptCore/ChangeLog:10
> > +        This space is maximum space needed for the callee registers that the LLInt  or baseline JIT
> 
> is maximum space -> is the maximum space

Fixed locally.

> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3295
> > +#endif
> 
> Isn't this the same as sizeof(Register)?

No, Register is our JS 64 bit register.

> > Source/JavaScriptCore/jit/GPRInfo.h:329
> > +    static const int numberLLIntBaselineCalleeSaveRegisters = 0;
> 
> I think numberOfLLIntBaselineCalleeSaveRegisters is nicer.

Fixed locally.
 
> > Source/JavaScriptCore/jit/RegisterSet.cpp:150
> > +    UNREACHABLE_FOR_PLATFORM();
> 
> What about MIPS and SH4?

Added.

[Michael Saboff]

Created attachment 257695 [details]
Patch with code to handle the DFG and OSR entry / exit.

[Basile Clement]

Comment on attachment 257695 [details]
Patch with code to handle the DFG and OSR entry / exit.

View in context: https://bugs.webkit.org/attachment.cgi?id=257695&action=review

LGTM with a few nits.

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:255
> -    jit.move(AssemblyHelpers::TrustedImmPtr(jit.codeBlock()->ownerExecutable()), GPRInfo::nonArgGPR0);
> -    osrWriteBarrier(jit, GPRInfo::nonArgGPR0, GPRInfo::nonArgGPR1);
> +    jit.move(AssemblyHelpers::TrustedImmPtr(jit.codeBlock()->ownerExecutable()), GPRInfo::argumentGPR1);
> +    osrWriteBarrier(jit, GPRInfo::argumentGPR1, GPRInfo::nonArgGPR0);

What is the reason for this change?

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:261
> -            jit.move(AssemblyHelpers::TrustedImmPtr(ownerExecutable), GPRInfo::nonArgGPR0); 
> -            osrWriteBarrier(jit, GPRInfo::nonArgGPR0, GPRInfo::nonArgGPR1);
> +            jit.move(AssemblyHelpers::TrustedImmPtr(ownerExecutable), GPRInfo::argumentGPR1);
> +            osrWriteBarrier(jit, GPRInfo::argumentGPR1, GPRInfo::nonArgGPR0);

Ditto.

> Source/JavaScriptCore/jit/GPRInfo.h:415
> -    static const GPRReg regCS4 = X86Registers::edi; // regT4
> +    static const GPRReg regCS4 = X86Registers::edi; // regT6

This is regT4. edi is regT6 on X86, but regT4 on X86_WIN.

> Source/JavaScriptCore/jit/GPRInfo.h:445
> +    static const int numberOfLLIntBaselineCalleeSaveRegisters = 5; // rbx, rdi, rsi, r14 & r15

We don't use rsi, saving rbx, rdi, r14 and r15 should be enough.

> Source/JavaScriptCore/jit/GPRInfo.h:635
> +    static const int numberCalleeSaveRegisters = 3;

This should be 2. We only use x27 and x28 in the DFG.

> Source/JavaScriptCore/jit/RegisterSaveMap.cpp:47
> +    HashMap<unsigned, unsigned, WTF::IntHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned> >::const_iterator iter = m_indexForRegister.find(reg.index());

Maybe use auto iter = ... instead?

> Source/JavaScriptCore/jit/RegisterSaveMap.h:51
> +    HashMap<unsigned, unsigned, WTF::IntHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned> > m_indexForRegister;

Maybe using a Vector<unsigned, MacroAssembler::numberOfRegisters()> would be better here? It requires making numerOfRegisters() and related functions constexpr, but that looks like a desirable thing anyway.

> Source/JavaScriptCore/jit/RegisterSet.cpp:160
> +RegisterSet RegisterSet::allVMCalleeSaveRegisters()
> +{
> +    RegisterSet result;
> +#if CPU(X86)
> +#elif CPU(X86_64)
> +    result.set(GPRInfo::regCS0);
> +    result.set(GPRInfo::regCS1);
> +    result.set(GPRInfo::regCS2);
> +#if !OS(WINDOWS)
> +    ASSERT(GPRInfo::regCS3 == GPRInfo::tagTypeNumberRegister);
> +    ASSERT(GPRInfo::regCS4 == GPRInfo::tagMaskRegister);
> +    result.set(GPRInfo::regCS3);
> +    result.set(GPRInfo::regCS4);
> +#else
> +    result.set(GPRInfo::regCS3);
> +    result.set(GPRInfo::regCS4);
> +    ASSERT(GPRInfo::regCS5 == GPRInfo::tagTypeNumberRegister);
> +    ASSERT(GPRInfo::regCS6 == GPRInfo::tagMaskRegister);
> +    result.set(GPRInfo::regCS5);
> +    result.set(GPRInfo::regCS6);
> +#endif
> +#elif CPU(ARM_THUMB2)
> +#elif CPU(ARM_TRADITIONAL)
> +#elif CPU(ARM64)
> +    result.set(ARM64Registers::x26);
> +    ASSERT(GPRInfo::regCS0 == GPRInfo::tagTypeNumberRegister);
> +    ASSERT(GPRInfo::regCS1 == GPRInfo::tagMaskRegister);
> +    result.set(GPRInfo::regCS0);
> +    result.set(GPRInfo::regCS1);
> +#elif CPU(MIPS)
> +#elif CPU(SH4)
> +#else
> +    UNREACHABLE_FOR_PLATFORM();
> +#endif
> +    return result;
> +}

Couldn't this be implemented as baselineCalleeSaveRegisters().merge(dfgCalleeSaveRegisters()); ? Seems like it would simplify future changes.

> Source/JavaScriptCore/llint/LLIntData.cpp:141
> +    ASSERT(CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters() == 5);

Cf above, this should be 4.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:226
> +    const CalleeSaveSpaceAsVirtualRegisters = 5

Ditto.

[Michael Saboff]

(In reply to comment #5)
> Comment on attachment 257695 [details]
> Patch with code to handle the DFG and OSR entry / exit.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257695&action=review
> 
> LGTM with a few nits.
> 
> > Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:255
> > -    jit.move(AssemblyHelpers::TrustedImmPtr(jit.codeBlock()->ownerExecutable()), GPRInfo::nonArgGPR0);
> > -    osrWriteBarrier(jit, GPRInfo::nonArgGPR0, GPRInfo::nonArgGPR1);
> > +    jit.move(AssemblyHelpers::TrustedImmPtr(jit.codeBlock()->ownerExecutable()), GPRInfo::argumentGPR1);
> > +    osrWriteBarrier(jit, GPRInfo::argumentGPR1, GPRInfo::nonArgGPR0);
> 
> What is the reason for this change?

GPRInfo::nonArgGPR1 is rbx on X86_64, which is a callee save.  I found this by looking through generated assembly language for callee save use.  I changed nonArgGPR0 to argumentGPR1 as it is the first argument and to free up nonArgGPR0.
 
> > Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:261
> > -            jit.move(AssemblyHelpers::TrustedImmPtr(ownerExecutable), GPRInfo::nonArgGPR0); 
> > -            osrWriteBarrier(jit, GPRInfo::nonArgGPR0, GPRInfo::nonArgGPR1);
> > +            jit.move(AssemblyHelpers::TrustedImmPtr(ownerExecutable), GPRInfo::argumentGPR1);
> > +            osrWriteBarrier(jit, GPRInfo::argumentGPR1, GPRInfo::nonArgGPR0);
> 
> Ditto.

See above

> > Source/JavaScriptCore/jit/GPRInfo.h:415
> > -    static const GPRReg regCS4 = X86Registers::edi; // regT4
> > +    static const GPRReg regCS4 = X86Registers::edi; // regT6
> 
> This is regT4. edi is regT6 on X86, but regT4 on X86_WIN.

I restored this.

> > Source/JavaScriptCore/jit/GPRInfo.h:445
> > +    static const int numberOfLLIntBaselineCalleeSaveRegisters = 5; // rbx, rdi, rsi, r14 & r15
> 
> We don't use rsi, saving rbx, rdi, r14 and r15 should be enough.

I made changes to eliminate rbi as a X86 Windows callee save for the LLInt and baseline JIT.

> > Source/JavaScriptCore/jit/GPRInfo.h:635
> > +    static const int numberCalleeSaveRegisters = 3;
> 
> This should be 2. We only use x27 and x28 in the DFG.

This needs to be number of registers in the union of callee saves for all VMs.  Since the LLInt uses 3 on ARM64, this needs to be 3.  It is used to size the callee saves buffer in the VM.

> > Source/JavaScriptCore/jit/RegisterSaveMap.cpp:47
> > +    HashMap<unsigned, unsigned, WTF::IntHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned> >::const_iterator iter = m_indexForRegister.find(reg.index());
> 
> Maybe use auto iter = ... instead?

But that is way too easy to read!!! (I changed it.)

> > Source/JavaScriptCore/jit/RegisterSaveMap.h:51
> > +    HashMap<unsigned, unsigned, WTF::IntHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned> > m_indexForRegister;
> 
> Maybe using a Vector<unsigned, MacroAssembler::numberOfRegisters()> would be
> better here? It requires making numerOfRegisters() and related functions
> constexpr, but that looks like a desirable thing anyway.

According to http://blogs.msdn.com/b/vcblog/archive/2013/12/02/c-11-14-core-language-features-in-vs-2013-and-the-nov-2013-ctp.aspx, constexpr is not part of VS 2013.  At this point, I'm going to keep it as it is.

> > Source/JavaScriptCore/jit/RegisterSet.cpp:160
> > +RegisterSet RegisterSet::allVMCalleeSaveRegisters()
> > +{
> > +    RegisterSet result;
> > +#if CPU(X86)
> > +#elif CPU(X86_64)
> > +    result.set(GPRInfo::regCS0);
> > +    result.set(GPRInfo::regCS1);
> > +    result.set(GPRInfo::regCS2);
> > +#if !OS(WINDOWS)
> > +    ASSERT(GPRInfo::regCS3 == GPRInfo::tagTypeNumberRegister);
> > +    ASSERT(GPRInfo::regCS4 == GPRInfo::tagMaskRegister);
> > +    result.set(GPRInfo::regCS3);
> > +    result.set(GPRInfo::regCS4);
> > +#else
> > +    result.set(GPRInfo::regCS3);
> > +    result.set(GPRInfo::regCS4);
> > +    ASSERT(GPRInfo::regCS5 == GPRInfo::tagTypeNumberRegister);
> > +    ASSERT(GPRInfo::regCS6 == GPRInfo::tagMaskRegister);
> > +    result.set(GPRInfo::regCS5);
> > +    result.set(GPRInfo::regCS6);
> > +#endif
> > +#elif CPU(ARM_THUMB2)
> > +#elif CPU(ARM_TRADITIONAL)
> > +#elif CPU(ARM64)
> > +    result.set(ARM64Registers::x26);
> > +    ASSERT(GPRInfo::regCS0 == GPRInfo::tagTypeNumberRegister);
> > +    ASSERT(GPRInfo::regCS1 == GPRInfo::tagMaskRegister);
> > +    result.set(GPRInfo::regCS0);
> > +    result.set(GPRInfo::regCS1);
> > +#elif CPU(MIPS)
> > +#elif CPU(SH4)
> > +#else
> > +    UNREACHABLE_FOR_PLATFORM();
> > +#endif
> > +    return result;
> > +}
> 
> Couldn't this be implemented as
> baselineCalleeSaveRegisters().merge(dfgCalleeSaveRegisters()); ? Seems like
> it would simplify future changes.

Creating a union makes sense only if we include the LLInt callee save registers.  There current;y isn't a LLInt callee save RegisterSet, it would need to be added.

> > Source/JavaScriptCore/llint/LLIntData.cpp:141
> > +    ASSERT(CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters() == 5);
> 
> Cf above, this should be 4.

I made this along with the change above.

> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:226
> > +    const CalleeSaveSpaceAsVirtualRegisters = 5
> 
> Ditto.

Ditto.

[Michael Saboff]

Committed r187639: <http://trac.webkit.org/changeset/187639>

[Csaba Osztrogonác]

Comment on attachment 257695 [details]
Patch with code to handle the DFG and OSR entry / exit.

Cleared review? from attachment 257695 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).