[webkit-reviews] review denied: [Bug 206611] [ARMv7][JIT] Implement checkpoint support : [Attachment 388590] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 206611: [ARMv7][JIT] Implement checkpoint support
https://bugs.webkit.org/show_bug.cgi?id=206611

Attachment 388590: Patch

https://bugs.webkit.org/attachment.cgi?id=388590&action=review




--- Comment #7 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 388590
  --> https://bugs.webkit.org/attachment.cgi?id=388590
Patch

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

Looks good, but put r- since I found some nits.

> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
> +	   if (sizeof(intptr_t) == 8)

This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame
and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:615
> +		       case InPair:

This only exists for 32_64. So, let's write

#if USE(JSVALUE32_64)
    case InPair:
#endif

instead of using `#else` of `#if USE(JSVALUE64)`.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:634
> +#endif
> +		       case BooleanDisplacedInJSStack:
> +		       case CellDisplacedInJSStack:
>		       case DisplacedInJSStack: {
>			   sideState->tmps[i] =
reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset];
>			   break;
>		       }
> +#if USE(JSVALUE32_64)
> +		       case UnboxedCellInGPR: {
> +			   EncodedValueDescriptor* valueDescriptor =
bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset);
> +			   sideState->tmps[i] = JSValue(JSValue::CellTag,
valueDescriptor->asBits.payload);
> +			   break;
> +		       }
> +
> +		       case UnboxedBooleanInGPR: {
> +			   sideState->tmps[i] =
jsBoolean(static_cast<bool>(tmpScratch[i + tmpOffset]));
> +			   break;
> +		       }
> +#endif

Having the same entries switched by `ifdef` is hard to read. Let's write it in
a simpler way, like this.

case UnboxedCellInGPR: {
#if USE(JSVALUE64)
    sideState->tmps[i] = reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset];
#else
    EncodedValueDescriptor* valueDescriptor =
bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset);
    sideState->tmps[i] = JSValue(JSValue::CellTag,
valueDescriptor->asBits.payload);
#endif
    break;
}