This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Varargs support
ClosedPublic

Authored by sunfish on Dec 4 2015, 7:59 AM.

Details

Summary

This is a preliminary patch for varargs support. It doesn't all work yet, and is quite untested. I'm just sending out the patch for comments.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish updated this revision to Diff 41880.Dec 4 2015, 7:59 AM
sunfish retitled this revision from to [WebAssembly] Varargs support.
sunfish updated this object.
sunfish set the repository for this revision to rL LLVM.
sunfish added subscribers: jfb, dschuff.
jfb added a comment.Dec 4 2015, 8:44 AM

Looks simpler than I thought it would be!

lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
321

This is just extra validation, unrelated to varargs? No need to split it out, I just want to make sure I'm not missing something.

347

This is pretty much the same approach as PNaCl, but done right. IIUC: varargs are effectively in a naturally-aligned struct at the top of stack, and the callee gets a pointer to the stark. Is that correct? If so, could the comment explain that part?

380

This just appends the fixed args, and doesn't append anything related to varags, correct?

464

Duplicated above. Factor out?

sunfish updated this revision to Diff 41894.Dec 4 2015, 9:49 AM

Roughly the same patch, but with some of the uninteresting bits split out, and some extra comments.

sunfish marked 3 inline comments as done.Dec 4 2015, 9:58 AM

One thing slightly different about this varargs implementation from the PNaCl LowerVarArgs pass is that this implementation doesn't pass a pointer argument; it just puts the varargs arguments at the top of the stack, so they are pointed to by the stack pointer (or from the callee's perspective, by the frame pointer). We could go either way, but I don't think it makes a difference in performance (and besides, this is varargs ;-)), and I think it's a little tidier this way.

lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
321

That's right. I decided to just split it out now, so it's out of the way.

347

Comments added.

380

I added a comment explaining this.

464

Done.

sunfish updated this revision to Diff 41943.Dec 4 2015, 2:47 PM
sunfish marked 3 inline comments as done.

Various fixes and cleanups. And add a testcase. Note that va_start and actual arguments don't work yet; they depend on frame pointer support. All the tests pass with this patch applied, so I think it's ready to land.

jfb added inline comments.Dec 4 2015, 3:00 PM
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
338

Add a comment string to the assert.

363

Ditto.

444

Typo?

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
207

Does this happen because of varargs? Could you comment.

test/CodeGen/WebAssembly/varargs.ll
126

No need for the attributes.

sunfish updated this revision to Diff 41950.Dec 4 2015, 3:12 PM

Update for review comments.

sunfish marked 5 inline comments as done.Dec 4 2015, 3:13 PM

Review comments addressed.

jfb accepted this revision.Dec 4 2015, 3:14 PM
jfb added a reviewer: jfb.

lgtm

This revision is now accepted and ready to land.Dec 4 2015, 3:14 PM
This revision was automatically updated to reflect the committed changes.