Bug 78612 - Implement fast path for op_new_array in the baseline JIT
Summary: Implement fast path for op_new_array in the baseline JIT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 78610
Blocks: 79055
  Show dependency treegraph
 
Reported: 2012-02-14 09:42 PST by Mark Hahnenberg
Modified: 2012-02-26 15:17 PST (History)
2 users (show)

See Also:


Attachments
Patch (17.37 KB, patch)
2012-02-20 16:29 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Bencher results (7.08 KB, text/plain)
2012-02-20 18:22 PST, Mark Hahnenberg
no flags Details
Patch (18.47 KB, patch)
2012-02-21 15:01 PST, Mark Hahnenberg
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2012-02-14 09:42:39 PST
Currently, op_new_array simply does a stub call out to C++ code. With the new copying collector, we should be able to implement an inline asm fast path for op_new_array.
Comment 1 Mark Hahnenberg 2012-02-20 16:29:27 PST
Created attachment 127868 [details]
Patch
Comment 2 Filip Pizlo 2012-02-20 16:37:03 PST
Comment on attachment 127868 [details]
Patch

I can dig it.  But check performance.
Comment 3 Mark Hahnenberg 2012-02-20 18:22:32 PST
Created attachment 127886 [details]
Bencher results
Comment 4 WebKit Review Bot 2012-02-20 19:20:27 PST
Comment on attachment 127868 [details]
Patch

Clearing flags on attachment: 127868

Committed r108291: <http://trac.webkit.org/changeset/108291>
Comment 5 WebKit Review Bot 2012-02-20 19:20:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Filip Pizlo 2012-02-20 22:20:58 PST
I think you broke 32-bit.
Comment 7 Filip Pizlo 2012-02-20 22:27:24 PST
Yeah you totally broke 32-bit. ;-)  I'm going to roll this out for now since it's blocking the interpreter.
Comment 8 Filip Pizlo 2012-02-20 22:31:32 PST
These are the failures.  Looks like you need to be careful about the JIT code you emit on 32-bit.  In particular, you can't copy JSValues around using Ptr; you have to use two 32s.



** Danger, Will Robinson! Danger! The following failures have been introduced:
	ecma/Date/15.9.3.1-1.js
	ecma/Date/15.9.3.1-2.js
	ecma/Date/15.9.3.1-3.js
	ecma/Date/15.9.3.1-4.js
	ecma/Date/15.9.3.1-5.js
	ecma/Date/15.9.3.2-4.js
	ecma/Date/15.9.3.2-5.js
	ecma/Date/15.9.3.8-2.js
	ecma/Date/15.9.3.8-3.js
	ecma/Date/15.9.3.8-4.js
	ecma/Date/15.9.3.8-5.js
	ecma/Date/15.9.4.2.js
	ecma_2/String/match-002.js
	ecma_3/Array/15.4.4.3-1.js
	ecma_3/Expressions/11.9.6-1.js
	ecma_3/RegExp/regress-85721.js
	js1_2/Array/splice2.js
	js1_2/regexp/alphanumeric.js
	js1_2/regexp/control_characters.js
	js1_2/regexp/digit.js
	js1_2/regexp/vertical_bar.js
	js1_2/regexp/whitespace.js
	js1_2/regress/regress-7703.js
	js1_5/Expressions/regress-96526-argsub.js
	js1_5/Expressions/regress-96526-delelem.js
	js1_5/Expressions/regress-96526-noargsub.js
	js1_5/GetSet/getset-004.js
	js1_5/GetSet/getset-005.js
	js1_5/Regress/regress-146596.js

29 regressions found.
Comment 9 Filip Pizlo 2012-02-20 22:34:03 PST
Rolled out in http://trac.webkit.org/changeset/108307
Comment 10 Mark Hahnenberg 2012-02-21 15:01:42 PST
Created attachment 128051 [details]
Patch
Comment 11 Filip Pizlo 2012-02-25 19:51:59 PST
Comment on attachment 128051 [details]
Patch

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

r=me if you fix the two issues.

> Source/JavaScriptCore/jit/JITInlineMethods.h:514
> +        loadPtr(Address(callFrameRegister, (valuesRegister + i) * sizeof(Register)), storagePtr);
> +        storePtr(storagePtr, Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i));
> +        loadPtr(Address(callFrameRegister, (valuesRegister + i) * sizeof(Register) + sizeof(uint32_t)), storagePtr);
> +        storePtr(storagePtr, Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i + sizeof(uint32_t)));

Minor nit:

We typically use load32 and store32 instead of loadPtr and storePtr for JSVALUE32_64.  I think historically that was required because we might compile the JSVALUE32_64 code for 64-bit.  We don't do that anymore, but I kind of like seeing load32/store32 because it reminds me that I'm looking at JSVALUE32_64 code rather than code for JSVALUE64.

> Source/JavaScriptCore/jit/JITInlineMethods.h:529
> +#if CPU(BIG_ENDIAN)
> +        store32(TrustedImm32(static_cast<int>(JSValue::EmptyValueTag)), Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i));
> +        storePtr(TrustedPtr(0), Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i + sizeof(uint32_t)));
> +#else
> +        storePtr(TrustedImmPtr(0), Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i));
> +        store32(TrustedImm32(static_cast<int>(JSValue::EmptyValueTag)), Address(storageResult, ArrayStorage::vectorOffset() + sizeof(WriteBarrier<Unknown>) * i + sizeof(uint32_t)));
> +#endif

You could get rid of the endian-casing if you use OBJECT_OFFSETOF into EncodedValueDescriptor.
Comment 12 Mark Hahnenberg 2012-02-26 15:17:09 PST
Committed r108681: <http://trac.webkit.org/changeset/108681>