Bug 181134 – [DFG] Unify ToNumber implementation in 32bit and 64bit by changing 32bit Int32Tag and LowestTag

• Status: REOPENED
• Component: New Bugs

[Yusuke Suzuki]

[DFG] Unify ToNumber implementation in 32bit and 64bit by changing 32bit Int32Tag and LowestTag

[Yusuke Suzuki]

Created attachment 330143 [details]
Patch

[Yusuke Suzuki]

Created attachment 330145 [details]
Patch

[Yusuke Suzuki]

Created attachment 330147 [details]
Patch

[EWS Watchlist]

Comment on attachment 330147 [details]
Patch

Attachment 330147 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5806635

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html

[EWS Watchlist]

Created attachment 330153 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 330201 [details]
Patch

[Yusuke Suzuki]

Created attachment 330202 [details]
Patch

[Guillaume Emont]

If it can wait until then, I'd be happy to try the patch on mips next week. If not, the test bot will after landing ;).

[Zan Dobersek]

Doesn't regress anything on ARMv7, I suppose it shouldn't on MIPS either.

[Yusuke Suzuki]

(In reply to Zan Dobersek from comment #9)
> Doesn't regress anything on ARMv7, I suppose it shouldn't on MIPS either.

Great!

[Guillaume Emont]

I just launched the tests on mips with this patch, I'll report back in about 7 hours.

[Mark Lam]

Comment on attachment 330202 [details]
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.

[Guillaume Emont]

Tests are still running on MIPS, but I seem to get a lot of assertion failures.
For instance (reproduced by hand for clarity):

# .vm/JavaScriptCore.framework/Resources/jsc .tests/ChakraCore.yaml/ChakraCore/t
est/JSON/syntaxError.js 
ASSERTION FAILED: size == this->size()
../../Source/JavaScriptCore/heap/IsoSubspace.cpp(65) : void* JSC::IsoSubspace::allocateNonVirtual(size_t, JSC::GCDeferralContext*, JSC::AllocationFailureMode)
Bus error

[Guillaume Emont]

Created attachment 330484 [details]
Partial test results

I am attaching the partial results of the ongoing test run with the patch, which include thousands of bus errors, which I think might all be assertion errors.

[Yusuke Suzuki]

(In reply to Guillaume Emont from comment #14)
> Created attachment 330484 [details]
> Partial test results
> 
> I am attaching the partial results of the ongoing test run with the patch,
> which include thousands of bus errors, which I think might all be assertion
> errors.

Hmm, I think this is not directly related to this patch.
I've just built x86 32bit debug build on my Mac and I cannot reproduce it.
I'll land this patch and watch the results in the bot.

Could you try the debug build without this patch?
I guess this assertion is introduced before this patch.

[Yusuke Suzuki]

Committed r226434: <https://trac.webkit.org/changeset/226434>

[Radar WebKit Bug Importer]

<rdar://problem/36312629>

[Guillaume Emont]

oops, indeed, get the same issue without the patch, will need to figure out what it is.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 181322

[Guillaume Emont]

What are the test failures on 32-bit?

[Guillaume Emont]

(In reply to Guillaume Emont from comment #20)
> What are the test failures on 32-bit?

I just ran the tests on mips with the patch, and I got 0 test failure.