Bug 203861

Summary: Use fewer virtual registers in Wasm LLInt
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Description Tadeu Zagallo 2019-11-05 13:45:20 PST
...
Comment 1 Tadeu Zagallo 2019-11-05 13:54:20 PST
Created attachment 382845 [details]
Patch
Comment 2 Saam Barati 2019-11-06 16:41:08 PST
This implementation worries me since it relies on the FunctionParser being written in a very specific way. Can we make it more robust somehow? Like what if you do this for some opcode where the function parser wants to re-use a stack slot internally for some reason to test it again?
Comment 3 Saam Barati 2019-11-07 18:24:36 PST
Comment on attachment 382845 [details]
Patch

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

> Source/JavaScriptCore/llint/WebAssembly.asm:611
> +        loadq -offset - 8 - CalleeSaveSpaceAsVirtualRegisters * 8[cfr], gpr
>      end)
>      forEachArgumentFPR(macro (offset, fpr)
> -        loadd offset[ws1], fpr
> +        loadd -offset - 8 - CalleeSaveSpaceAsVirtualRegisters * 8[cfr], fpr

This works because of the code you have in callInformationFor, right?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:354
> +        for (uint32_t i = gprCount + fprCount; i--;)
> +            registers.append(new RegisterID(::JSC::virtualRegisterForLocal(numberOfLLIntCalleeSaveRegisters + i)));

this looks wrong. How do we even know we have this much stack space? Should be easy to add a test I think.
Comment 4 Tadeu Zagallo 2019-11-07 18:26:37 PST
Comment on attachment 382845 [details]
Patch

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

>> Source/JavaScriptCore/llint/WebAssembly.asm:611
>> +        loadd -offset - 8 - CalleeSaveSpaceAsVirtualRegisters * 8[cfr], fpr
> 
> This works because of the code you have in callInformationFor, right?

No, this work because of the code in LLIntGenerator::addArguments.

>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:354
>> +            registers.append(new RegisterID(::JSC::virtualRegisterForLocal(numberOfLLIntCalleeSaveRegisters + i)));
> 
> this looks wrong. How do we even know we have this much stack space? Should be easy to add a test I think.

Because we always allocate space to spill all the registers at entry
Comment 5 Saam Barati 2019-11-07 18:28:43 PST
Comment on attachment 382845 [details]
Patch

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

>>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:354
>>> +            registers.append(new RegisterID(::JSC::virtualRegisterForLocal(numberOfLLIntCalleeSaveRegisters + i)));
>> 
>> this looks wrong. How do we even know we have this much stack space? Should be easy to add a test I think.
> 
> Because we always allocate space to spill all the registers at entry

might be worth a comment
Comment 6 Tadeu Zagallo 2019-11-07 19:00:08 PST
Comment on attachment 382845 [details]
Patch

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

Thanks for the review!

>>>> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:354
>>>> +            registers.append(new RegisterID(::JSC::virtualRegisterForLocal(numberOfLLIntCalleeSaveRegisters + i)));
>>> 
>>> this looks wrong. How do we even know we have this much stack space? Should be easy to add a test I think.
>> 
>> Because we always allocate space to spill all the registers at entry
> 
> might be worth a comment

sounds good, I'll add it before landing.
Comment 7 Tadeu Zagallo 2019-11-07 19:02:29 PST
Created attachment 383104 [details]
Patch for landing
Comment 8 Tadeu Zagallo 2019-11-07 19:03:42 PST
Comment on attachment 383104 [details]
Patch for landing

Oops, the ChangeLog got messed when rebasing
Comment 9 Tadeu Zagallo 2019-11-07 19:10:32 PST
Created attachment 383106 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-11-07 19:55:55 PST
Comment on attachment 383106 [details]
Patch for landing

Clearing flags on attachment: 383106

Committed r252231: <https://trac.webkit.org/changeset/252231>
Comment 11 WebKit Commit Bot 2019-11-07 19:55:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-11-07 19:56:20 PST
<rdar://problem/57009335>