[webkit-reviews] review denied: [Bug 125881] frameRegisterCount() should include maxFrameExtentForSlowPathCall. : [Attachment 219463] the patch.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 125881: frameRegisterCount() should include maxFrameExtentForSlowPathCall.
https://bugs.webkit.org/show_bug.cgi?id=125881

Attachment 219463: the patch.
https://bugs.webkit.org/attachment.cgi?id=219463&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219463&action=review


> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3378
> +	   return
jitCode()->dfgCommon()->frameRegisterCountWithSlowPathFrameExtent();
> +
>      case JITCode::FTLJIT:
>	   return jitCode()->dfgCommon()->frameRegisterCount;

This smells fishy.

I don't think the DFG wants a frameRegisterCount, and a separate
frameRegisterCountWithSlowPathFrameExtent helper function, where the former is
an invitation to do things wrong, and the latter is the right answer. I think
we just want to put the right answer into frameRegisterCount. My understanding
is that's the whole point of that data member.

frameRegisterCount is already initialized for us. The bug you need to fix is
that DFG::Graph::frameRegisterCount() does not add in the max slow path thingy.
(This also means that the DFG is using the wrong value for SP right now.)

> Source/JavaScriptCore/jit/JIT.cpp:785
> +    ASSERT(!(codeBlock->m_numCalleeRegisters & 1));

Let's remove this.

> Source/JavaScriptCore/jit/JIT.h:250
> +	       return -virtualRegisterForLocal(frameRegisterCountFor(codeBlock)
- 1).offset() * sizeof(Register);

Instead of frameRegisterCountInBytes, it look like what we need is for
VirtualRegister to provide an offsetInBytes() function.

We don't want to directly encode the "-" here, because that makes an assumption
about the number line used by VirtualRegister, and the whole point of
virtualRegisterForLocal is to encapsulate that number line.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:640
> +    size_t frameExtent = JIT::frameRegisterCountInBytesFor(codeBlock());

Let's rename the LHS to match the RHS.