Bug 141183

Created attachment 245916 [details] work in progress This fixes all of the 64-bit paths. Still need to do 32-bit DFG.

Created attachment 245917 [details] the patch

Attachment 245917 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2007: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 245918 [details] the patch Fix style and the wording of the ChangeLog

Comment on attachment 245918 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245918&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4356 > Imm32(node->origin.semantic.inlineCallFrame->arguments.size()))); I think you need to -1 from arguments.size() here. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4400 > + m_jit.offsetOfArgumentsIncludingThis(node->origin.semantic) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.tag) + sizeof(EncodedJSValue)), Per our offline discussion, change offsetOfArgumentsIncludingThis() into offsetOfArguments() and hence, no need for the "+ sizeof(EncodedJSValue))" thereafter. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4432 > Imm32(node->origin.semantic.inlineCallFrame->arguments.size()))); Ditto. Need to -1 from arguments.size() here. > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1053 > + loadPtr(BaseIndex(callFrameRegister, regT2, TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.payload) + CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT0); > + loadPtr(BaseIndex(callFrameRegister, regT2, TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.tag) + CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT1); Instead of CallFrame::thisArgumentOffset(), you can use CallFrame::argumentOffset(0) here, and do away with the "+ sizeof(Register)" adjustments. > Source/JavaScriptCore/jit/JITOpcodes.cpp:928 > + load64(BaseIndex(callFrameRegister, regT1, TimesEight, CallFrame::thisArgumentOffset() * static_cast<int>(sizeof(Register)) + sizeof(Register)), regT0); Ditto. Use CallFrame::argumentOffset(0) and remove adjustment. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1621 > + loadi ThisArgumentOffset + 8 + TagOffset[cfr, t2, 8], t0 > + loadi ThisArgumentOffset + 8 + PayloadOffset[cfr, t2, 8], t1 I think it'll be clearer if you define a FirstArgumentOffset in LowLevelInterpreter.asm, and use that here instead of using ThisArgumentOffset + 8. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1480 > + loadq ThisArgumentOffset + 8[cfr, t2, 8], t0 Ditto. Use FirstArgumentOffset.

Created attachment 245920 [details] the patch Address Mark's comments

Comment on attachment 245920 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245920&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1617 > + subi 1, t2 Shouldn't this be t1?

Comment on attachment 245920 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245920&action=review >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1617 >> + subi 1, t2 > > Shouldn't this be t1? Yes. :-) I just fixed that locally. Magically, tests started passing.

Comment on attachment 245920 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=245920&action=review r=me with remaining issues addressed. > Source/JavaScriptCore/jit/AssemblyHelpers.h:609 > + return CallFrame::argumentOffsetIncludingThis(1) * sizeof(Register); You can make this CallFrame::argumentOffset(0) instead of CallFrame::argumentOffsetIncludingThis(1).

(In reply to comment #9) > Comment on attachment 245920 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245920&action=review > > r=me with remaining issues addressed. > > > Source/JavaScriptCore/jit/AssemblyHelpers.h:609 > > + return CallFrame::argumentOffsetIncludingThis(1) * sizeof(Register); > > You can make this CallFrame::argumentOffset(0) instead of > CallFrame::argumentOffsetIncludingThis(1). Thanks for the feedback, I just made this change locally.

Landed in http://trac.webkit.org/changeset/179538