Bug 123182 – Change local variable register allocation to start at offset -1

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

[Michael Saboff]

Local variables and temporaries in the JavaScript engine start at offset or slot 0 from the CallFrame location.  To align with native calling conventions, the frame pointer typically points at either the previous frame pointer or return PC.  Therefore local need to start at offset -1 from the call frame register.

[Michael Saboff]

Created attachment 214898 [details]
Work in progress

Passes JSC tests using LLInt + baseline JIT on 64 bit. Runs all but on 32 bit test using LLInt + baseline JIT.  It crashes in js/script-tests/function-apply-aliased.js.

[Michael Saboff]

Created attachment 215080 [details]
Patch

The definition and use of numCalleeRegistersIncludingSkippedSlot0() will need to change when this patch is reconciled with the relayout of CallFrame.

[Geoffrey Garen]

Comment on attachment 215080 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.h:350
> +    int numCalleeRegistersIncludingSkippedSlot0() { return m_numCalleeRegisters + 1; }

We want the call frame pointer to point to the first call frame header slot, with the first local below it. This skipped slot 0 design logically points the call frame header at the first local slot, and then avoids allocating a virtual register to the first local slot. That's not quite what we want.

Instead, you should adjust the call frame pointer to point to the first call frame header slot.

> Source/JavaScriptCore/bytecode/VirtualRegister.h:77
> +    static int localToOperand(int local) { return -1-local; }
> +    static int operandToLocal(int operand) { return -1-operand; }

Spaces around "-" please.

[Michael Saboff]

Created attachment 215336 [details]
Patch - Only DFG inlining remaining

[Michael Saboff]

Created attachment 215421 [details]
Patch

Has updates suggested from original review.

[WebKit Commit Bot]

Attachment 215421 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/VirtualRegister.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp', u'Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/JSStack.h', u'Source/JavaScriptCore/interpreter/JSStackInlines.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp', u'Source/JavaScriptCore/llint/LLIntData.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter64.asm', u'Source/JavaScriptCore/runtime/CommonSlowPaths.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj']" exit_code: 1
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/JavaScriptCore/llint/LLIntData.cpp:79:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

(In reply to comment #6)
> Attachment 215421 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/VirtualRegister.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp', u'Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/JSStack.h', u'Source/JavaScriptCore/interpreter/JSStackInlines.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp', u'Source/JavaScriptCore/llint/LLIntData.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter64.asm', u'Source/JavaScriptCore/runtime/CommonSlowPaths.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj']" exit_code: 1
> Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> Source/JavaScriptCore/llint/LLIntData.cpp:79:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> Total errors found: 2 in 23 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I will remove the inadvertent changes to Tools/*.  (Thanks Xcode).

The style error in LLIntData.cpp follows existing pattern, but I can change the ASSERT().

[Geoffrey Garen]

Comment on attachment 215421 [details]
Patch

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

> Source/JavaScriptCore/bytecode/VirtualRegister.h:77
> +    static int localToOperand(int local) { return -1-local; }
> +    static int operandToLocal(int operand) { return -1-operand; }

Spaces around "-", please.

Looks like this abstraction worked out really well. It's nice that we could avoid making any changes to JSActivation, Arguments, and BytecodeGenerator in this patch.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:320
> +    end = callFrame->registers() + JSStack::ThisArgument;
> +    while (it >= end) {

In C++, "end" means "past the end", so we should keep >, and change the value of end.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:936
> +    if (((intptr_t)callFrame & 0xfff) == 0xce8)

Please remove.

> Source/JavaScriptCore/interpreter/JSStack.h:56
> +            ThisArgument = 6,

Let's use avoid duplication with CallFrame:s_thisArgumentOffset and CallFrame::s_firstArgumentOffset, and let's make sure these constants live next to each other. If we want them in JSStack.h, I'd call them:

FirstArgument = 7
ThisArgument = 6

> Source/JavaScriptCore/runtime/JSGlobalObject.h:149
>      // Add one so we don't need to index with -1 to get current frame pointer.
>      // An index of -1 is an error for some compilers.
> -    Register m_globalCallFrame[JSStack::CallFrameHeaderSize + 1];
> +    Register m_globalCallFrame[JSStack::CallFrameHeaderSize];

Please update this comment, or remove it.

[Michael Saboff]

(In reply to comment #8)
> (From update of attachment 215421 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215421&action=review
> 
> > Source/JavaScriptCore/bytecode/VirtualRegister.h:77
> > +    static int localToOperand(int local) { return -1-local; }
> > +    static int operandToLocal(int operand) { return -1-operand; }
> 
> Spaces around "-", please.
> 
> Looks like this abstraction worked out really well. It's nice that we could avoid making any changes to JSActivation, Arguments, and BytecodeGenerator in this patch.
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:320
> > +    end = callFrame->registers() + JSStack::ThisArgument;
> > +    while (it >= end) {
> 
> In C++, "end" means "past the end", so we should keep >, and change the value of end.
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:936
> > +    if (((intptr_t)callFrame & 0xfff) == 0xce8)
> 
> Please remove.
> 
> > Source/JavaScriptCore/interpreter/JSStack.h:56
> > +            ThisArgument = 6,
> 
> Let's use avoid duplication with CallFrame:s_thisArgumentOffset and CallFrame::s_firstArgumentOffset, and let's make sure these constants live next to each other. If we want them in JSStack.h, I'd call them:
> 
> FirstArgument = 7
> ThisArgument = 6
> 
> > Source/JavaScriptCore/runtime/JSGlobalObject.h:149
> >      // Add one so we don't need to index with -1 to get current frame pointer.
> >      // An index of -1 is an error for some compilers.
> > -    Register m_globalCallFrame[JSStack::CallFrameHeaderSize + 1];
> > +    Register m_globalCallFrame[JSStack::CallFrameHeaderSize];
> 
> Please update this comment, or remove it.

I made the suggested changes.  I also made some fixes to Interpreter::dumpRegisters() to test the changes.  There had been some bit rot.

[Michael Saboff]

Committed r158237: <http://trac.webkit.org/changeset/158237>