[webkit-reviews] review granted: [Bug 78612] Implement fast path for op_new_array in the baseline JIT : [Attachment 128051] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted  review:
Bug 78612: Implement fast path for op_new_array in the baseline JIT
https://bugs.webkit.org/show_bug.cgi?id=78612

Attachment 128051: Patch
https://bugs.webkit.org/attachment.cgi?id=128051&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.