Instead of passing varargs directly on the user stack, allocate a buffer in
the caller's stack frame and pass a pointer to it. This simplifies the C
ABI (e.g. non-C callers of C functions do not need to use C's user stack if
they have their own mechanism) and allows further optimizations in the future
(e.g. fewer functions may need to use the stack).
Details
Diff Detail
Event Timeline
A couple of questions:
lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp | ||
---|---|---|
149 ↗ | (On Diff #47381) | I don't really understand what this assert is for. Is it OK to remove it? Should I put in something else? |
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp | ||
628 | One issue I currently have is that if ARGUMENTS is not live-in to the current block, then I get machineinstr verifier error because adding this reference does not add ARGUMENTS to the live-ins for the block. I could add it manually here, but I don't see a good way to get the current MBB from the current SelectionDAG, so maybe that's not the right thing to do. Any ideas? |
lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp | ||
---|---|---|
149 ↗ | (On Diff #47381) | There are two categories of instructions with variadic arguments: calls and tableswitch. Call arguments are all registers from LLVM's perspective (if they are constants, they get loaded into "registers" with i32.const etc.). Tableswitch arguments are all immediates. We use the WebAssemblyII::VariableOpIsImmediate flag to indicate to naive code which kind of operand to expect in the variadic portion of an instruction. This assert is just asserting that if that flag is set, and we're in the variadic portion, that we are actually seeing an immediate. This assert is relevant, so we shouldn't remove it. |
lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp | ||
51 | Why did MFI->isFrameAddressTaken() go away here? | |
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp | ||
628 | In the entry block, when we're creating the ARGUMENTs for the fixed arguments, we should create the ARGUMENT for the vararg pointer too, and remember which virtual register we put it in. LowerVASTART should then be able to just read this register, without needing ARGUMENTS live to wherever it happens to be. |
I still want to add more testing but here's round 2 addressing the comments, and fixing the problem I mentioned with LowerVASTART above.
lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp | ||
---|---|---|
149 ↗ | (On Diff #47381) | Turns out this was only relevant for a previous version of this change and isn't necessary after I fixed a bug; I put the assert back. |
lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp | ||
51 | Because I added an assert on line 77 that the frame address is never taken. This change means that arguments are never passed on the stack, and even if there's a frame pointer or (I think?) stack realignment, the frame address still shouldn't be taken. (Unless there's use of e.g. __builtin_frameaddress or something? but maybe we don't want to support that anyway?) | |
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp | ||
413 | Moved the line with the rest of the output arg code and updated comment. | |
test/CodeGen/WebAssembly/varargs.ll | ||
141 | This was an attempt to tickle the bug I mentioned above, and wasn't meant to be checked in. Another test forthcoming. |
test/CodeGen/WebAssembly/varargs.ll | ||
---|---|---|
141 | Test is now fixed and checks the right thing. |
Looks good!
lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp | ||
---|---|---|
51 | I think we should support __builtin_frame_address(0), but if you want to remove the MFI->isFrameAddressTaken() check for now, it'd be good to add an assert right here, in this function, so that if we ever set it we can be quickly pointed to the place where it needs to be handled. | |
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp | ||
364 | Instead of hard-coding a 16 here, let's use the DataLayout's getStackAlignment(). |
Why did MFI->isFrameAddressTaken() go away here?