[DFG] Unify ToNumber implementation in 32bit and 64bit by changing 32bit Int32Tag and LowestTag
Created attachment 330143 [details] Patch
Created attachment 330145 [details] Patch
Created attachment 330147 [details] Patch
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
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
Created attachment 330201 [details] Patch
Created attachment 330202 [details] Patch
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 ;).
Doesn't regress anything on ARMv7, I suppose it shouldn't on MIPS either.
(In reply to Zan Dobersek from comment #9) > Doesn't regress anything on ARMv7, I suppose it shouldn't on MIPS either. Great!
I just launched the tests on mips with this patch, I'll report back in about 7 hours.
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.
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
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.
(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.
Committed r226434: <https://trac.webkit.org/changeset/226434>
<rdar://problem/36312629>
oops, indeed, get the same issue without the patch, will need to figure out what it is.
Re-opened since this is blocked by bug 181322
What are the test failures on 32-bit?
(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.