[webkit-reviews] review granted: [Bug 117103] DFG should populate frame bytecodeOffsets on OSR exit : [Attachment 203479] the patch.

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 117103: DFG should populate frame bytecodeOffsets on OSR exit
https://bugs.webkit.org/show_bug.cgi?id=117103

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

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203479&action=review


R=me with those changes.

> Source/JavaScriptCore/bytecode/Opcode.h:282
>  inline size_t opcodeLength(OpcodeID opcode)
>  {
> -    switch (opcode) {
> -#define OPCODE_ID_LENGTHS(id, length) case id: return OPCODE_LENGTH(id);
> -	    FOR_EACH_OPCODE_ID(OPCODE_ID_LENGTHS)
> -#undef OPCODE_ID_LENGTHS
> -    }
> -    RELEASE_ASSERT_NOT_REACHED();
> -    return 0;
> +    return opcodeLengths[opcode];
>  }

Why?

The intuition for this method is that it gives the compiler a better
opportunity to constant fold.  We were expecting that if you call
opcodeLength() with a constant then the call will be folded to a constant.  I
think that's harder if you do an array lookup instead.

If you have a good reason for this change and it doesn't affect performance,
then I'm fine with it - I just want to make sure that you had a reason for this
change.

> Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp:41
> +    return opcodeLength(opcodeID);

This call is another example of where a switch is better than an array lookup:
with a switch statement, a C++ compiler backend will either choose to use a
lookup table (transforming a switch with all cases that return a constant into
an array lookup) or branches depending on what it thinks is most profitable for
the given target.

> Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp:657
> +	   unsigned returnBytecodeIndex = inlineCallFrame->caller.bytecodeIndex

> +	       + opcodeLengthForBytecodeIndex(m_jit.vm(),
baselineCodeBlockForCaller, inlineCallFrame->caller.bytecodeIndex);

I would love to have this assert that the caller bytecode index is in fact a
call of some kind.  I would prefer a RELEASE_ASSERT.  Can you refiddle this
code to do such an assertion?  I know it's awkward since you've got this
helper, but the assertion is more important than having a nice helper.

> Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp:621
> +	   unsigned returnBytecodeIndex = inlineCallFrame->caller.bytecodeIndex

> +	       + opcodeLengthForBytecodeIndex(m_jit.vm(),
baselineCodeBlockForCaller, inlineCallFrame->caller.bytecodeIndex);

Ditto.