Bug 190932

Summary: Consider removing double load for accessing the instructions from LLInt
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch mark.lam: review+

Description Tadeu Zagallo 2018-10-25 16:34:55 PDT
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.
Comment 1 Yusuke Suzuki 2018-11-12 10:27:35 PST
Created attachment 354567 [details]
Patch
Comment 2 EWS Watchlist 2018-11-12 10:30:13 PST
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.
Comment 3 Yusuke Suzuki 2018-11-15 17:56:35 PST
Ping?
Comment 4 Yusuke Suzuki 2018-11-18 22:09:04 PST
Ping?
Comment 5 Mark Lam 2018-11-18 22:18:49 PST
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?
Comment 6 Yusuke Suzuki 2018-11-18 22:21:10 PST
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.
Comment 7 Mark Lam 2018-11-18 22:28:09 PST
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.
Comment 8 Yusuke Suzuki 2018-11-18 22:51:59 PST
Committed r238367: <https://trac.webkit.org/changeset/238367>
Comment 9 Radar WebKit Bug Importer 2018-11-18 22:53:45 PST
<rdar://problem/46161984>