Bug 203861 – Use fewer virtual registers in Wasm LLInt

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

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 382845 [details]
Patch

[Saam Barati]

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?

[Saam Barati]

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.

[Tadeu Zagallo]

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

[Saam Barati]

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

[Tadeu Zagallo]

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.

[Tadeu Zagallo]

Created attachment 383104 [details]
Patch for landing

[Tadeu Zagallo]

Comment on attachment 383104 [details]
Patch for landing

Oops, the ChangeLog got messed when rebasing

[Tadeu Zagallo]

Created attachment 383106 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 383106 [details]
Patch for landing

Clearing flags on attachment: 383106

Committed r252231: <https://trac.webkit.org/changeset/252231>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57009335>