...
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.
<rdar://problem/57009335>