[webkit-reviews] review granted: [Bug 228543] [JSC] load/store with BaseIndex is inefficient in ARM64 : [Attachment 434434] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 228543: [JSC] load/store with BaseIndex is inefficient in ARM64
https://bugs.webkit.org/show_bug.cgi?id=228543

Attachment 434434: Patch

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




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

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

r=me with suggestions.

> Source/JavaScriptCore/assembler/testmasm.cpp:4224
> +	   uint64_t array[] = { 1, 2, 3, 4, 5, };
> +	   CHECK_EQ(invoke<uint64_t>(test, array,
static_cast<UCPURegister>(3)), 3);

nit: can you add  INT64_MAX to each of the values in array so that this test
will also catch bugs where the load instruction doesn't load the full 8 bytes?

> Source/JavaScriptCore/assembler/testmasm.cpp:4234
> +	   uint64_t array[] = { 1, 2, 3, 4, 5, };
> +	   CHECK_EQ(invoke<uint64_t>(test, array,
static_cast<UCPURegister>(3)), 5);

Ditto.

> Source/JavaScriptCore/assembler/testmasm.cpp:4247
> +	   uint32_t array[] = { 1, 2, 3, 4, 5, };
> +	   CHECK_EQ(invoke<uint32_t>(test, array,
static_cast<UCPURegister>(3)), 3);

Ditto: add INT32_MAX.

> Source/JavaScriptCore/assembler/testmasm.cpp:4257
> +	   uint32_t array[] = { 1, 2, 3, 4, 5, };
> +	   CHECK_EQ(invoke<uint32_t>(test, array,
static_cast<UCPURegister>(3)), 5);

Ditto.

> Source/JavaScriptCore/assembler/testmasm.cpp:4269
> +	   uint16_t array[] = { 1, 2, 3, 4, 5, };
> +	   CHECK_EQ(invoke<uint32_t>(test, array,
static_cast<UCPURegister>(3)), 3);

Ditto: add INT16_MAX.

> Source/JavaScriptCore/assembler/testmasm.cpp:4279
> +	   uint16_t array[] = { 1, 2, 3, 4, static_cast<uint16_t>(-1), };
> +	   CHECK_EQ(invoke<uint32_t>(test, array,
static_cast<UCPURegister>(3)), 0xffff);

Ditto.

> Source/JavaScriptCore/assembler/testmasm.cpp:4291
> +	   uint16_t array[] = { 1, 2, 3, 4, 5, };
> +	   CHECK_EQ(invoke<uint32_t>(test, array,
static_cast<UCPURegister>(3)), 3);

nit: add (INT16_MAX - 10)?

> Source/JavaScriptCore/assembler/testmasm.cpp:4301
> +	   uint16_t array[] = { 1, 2, 3, 4, static_cast<uint16_t>(-1), };
> +	   CHECK_EQ(invoke<uint32_t>(test, array,
static_cast<UCPURegister>(3)), static_cast<uint32_t>(-1));

nit: add (INT16_MAX - 10) but make leave the last one as -1?

> Source/JavaScriptCore/assembler/testmasm.cpp:4406
> +	   invoke<void>(test, array, 3, 42);
> +	   CHECK_EQ(array[2], 42);

nit: Add INT64_MAX to 42.

> Source/JavaScriptCore/assembler/testmasm.cpp:4417
> +	   invoke<void>(test, array, 3, 42);
> +	   CHECK_EQ(array[4], 42);

Ditto.

> Source/JavaScriptCore/assembler/testmasm.cpp:4431
> +	   invoke<void>(test, array, 3, 42);
> +	   CHECK_EQ(array[2], 42);

nit: Add INT32_MAX to 42.

> Source/JavaScriptCore/assembler/testmasm.cpp:4442
> +	   invoke<void>(test, array, 3, 42);
> +	   CHECK_EQ(array[4], 42);

Ditto.

> Source/JavaScriptCore/assembler/testmasm.cpp:4455
> +	   invoke<void>(test, array, 3, 42);
> +	   CHECK_EQ(array[2], 42);

nit: Add INT16_MAX to 42.

> Source/JavaScriptCore/assembler/testmasm.cpp:4466
> +	   invoke<void>(test, array, 3, 42);
> +	   CHECK_EQ(array[4], 42);

Ditto.