This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Switch varags calling convention to use a register
ClosedPublic

Authored by dschuff on Feb 9 2016, 3:16 PM.

Details

Summary

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).

Diff Detail

Event Timeline

dschuff updated this revision to Diff 47381.Feb 9 2016, 3:16 PM
dschuff retitled this revision from to [WebAssembly] Switch varags calling convention to use a register.
dschuff updated this object.
dschuff added reviewers: sunfish, jfb.
dschuff added a subscriber: llvm-commits.

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?

sunfish added inline comments.Feb 9 2016, 4:14 PM
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.

jfb added inline comments.Feb 9 2016, 4:28 PM
lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
67

&& "what is this checking?"

lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
364

/*Alignment=*/16

413

This looks like an unfinished

test/CodeGen/WebAssembly/varargs.ll
141

?

dschuff updated this revision to Diff 47469.Feb 10 2016, 9:24 AM
dschuff marked 2 inline comments as done.
  • Address review comments

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.

dschuff updated this revision to Diff 47481.Feb 10 2016, 10:16 AM
  • Update basic block test
dschuff added inline comments.Feb 10 2016, 10:20 AM
test/CodeGen/WebAssembly/varargs.ll
141

Test is now fixed and checks the right thing.

jfb edited edge metadata.Feb 10 2016, 11:22 AM

lgtm

This revision was automatically updated to reflect the committed changes.
sunfish edited edge metadata.Feb 10 2016, 12:03 PM

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().

dschuff marked 2 inline comments as done.Feb 10 2016, 12:18 PM

addressed comments in rL260429