Bug 123182

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.

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.

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.

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

Created attachment 215421 [details] Patch Has updates suggested from original review.

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.

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

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.

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

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