[webkit-reviews] review granted: [Bug 181134] [DFG] Unify ToNumber implementation in 32bit and 64bit by changing 32bit Int32Tag and LowestTag : [Attachment 330202] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 181134: [DFG] Unify ToNumber implementation in 32bit and 64bit by changing
32bit Int32Tag and LowestTag
https://bugs.webkit.org/show_bug.cgi?id=181134

Attachment 330202: Patch

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




--- Comment #12 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 330202
  --> https://bugs.webkit.org/attachment.cgi?id=330202
Patch

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

r=me with fixes.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2647
> -	       JITCompiler::Jump isNumber = m_jit.branch32(JITCompiler::Below,
op1TagGPR, JITCompiler::TrustedImm32(JSValue::LowestTag + 1));
> +	       JITCompiler::Jump isDouble = m_jit.branch32(JITCompiler::Below,
op1TagGPR, JITCompiler::TrustedImm32(JSValue::LowestTag));

Nice.  Looks like you fixed a bug here.  The old code should have compared
against LowestTag instead of LowestTag + 1.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:788
>      // Note that the tempGPR is not used in 64-bit mode.

Please remove this obsolete comment.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:808
>      // Note that the tempGPR is not used in 64-bit mode.

Please remove this obsolete comment.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1499
>  .opPutByIdTypeCheckNumber:
> -    bib t2, LowestTag + 1, .opPutByIdDoneCheckingTypes
> +    bibeq t2, LowestTag, .opPutByIdDoneCheckingTypes

Nice. Looks like you fixed a bug here.

I think the 2 checks that precedes this check can be eliminated i.e. replace:

    # We are either Int32 or Number.
    bieq t1, PutByIdSecondaryTypeNumber, .opPutByIdTypeCheckNumber

    bieq t2, Int32Tag, .opPutByIdDoneCheckingTypes
    jmp .opPutByIdSlow

.opPutByIdTypeCheckNumber:
   bibeq t2, LowestTag, .opPutByIdDoneCheckingTypes

... with:
   bibeq t2, LowestTag, .opPutByIdDoneCheckingTypes

Please take a second look if that is true.