Summary: | Use fewer virtual registers in Wasm LLInt | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tadeu Zagallo <tzagallo> | ||||||||
Component: | JavaScriptCore | Assignee: | Tadeu Zagallo <tzagallo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Tadeu Zagallo
2019-11-05 13:45:20 PST
Created attachment 382845 [details]
Patch
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 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 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 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 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. Created attachment 383104 [details]
Patch for landing
Comment on attachment 383104 [details]
Patch for landing
Oops, the ChangeLog got messed when rebasing
Created attachment 383106 [details]
Patch for landing
Comment on attachment 383106 [details] Patch for landing Clearing flags on attachment: 383106 Committed r252231: <https://trac.webkit.org/changeset/252231> All reviewed patches have been landed. Closing bug. |