Bug 190932 – Consider removing double load for accessing the instructions from LLInt

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

https://bugs.webkit.org/show_bug.cgi?id=187373 changes CodeBlock::m_instructions from being an array of instructions to being a InstructionStream, which itself contains a Vector of instructions. Therefore, we need two loads to get to the actual Instruction*. The implementation was done this way to avoid copying the instructions once we finish writing to the stream after bytecode generation, but it might not be the best trade-off.

[Yusuke Suzuki]

Created attachment 354567 [details]
Patch

[EWS Watchlist]

Attachment 354567 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:373:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Ping?

[Yusuke Suzuki]

Ping?

[Mark Lam]

Comment on attachment 354567 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1818
> -    muli sizeof SimpleJumpTable, t3   # FIXME: would be nice to peephole this!
> +    muli sizeof SimpleJumpTable, t3

What's the peephole opportunity the comment was talking about, and is it invalid / inappropriate?  What is your reason for this change?

[Yusuke Suzuki]

Comment on attachment 354567 [details]
Patch

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1818
>> +    muli sizeof SimpleJumpTable, t3
> 
> What's the peephole opportunity the comment was talking about, and is it invalid / inappropriate?  What is your reason for this change?

This removal is because we already did this optimization in https://trac.webkit.org/changeset/237627/webkit.
This comment means that we can perform strength reduction to convert `muli` to `lshifti` if `sizeof SimpleJumpTable` is a power of two.

[Mark Lam]

Comment on attachment 354567 [details]
Patch

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

r=me.

>>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1818
>>> +    muli sizeof SimpleJumpTable, t3
>> 
>> What's the peephole opportunity the comment was talking about, and is it invalid / inappropriate?  What is your reason for this change?
> 
> This removal is because we already did this optimization in https://trac.webkit.org/changeset/237627/webkit.
> This comment means that we can perform strength reduction to convert `muli` to `lshifti` if `sizeof SimpleJumpTable` is a power of two.

OK, thanks.

[Yusuke Suzuki]

Committed r238367: <https://trac.webkit.org/changeset/238367>

[Radar WebKit Bug Importer]

<rdar://problem/46161984>