Page MenuHomePhabricator

[WebAssembly] Implement __builtin_return_address for emscripten
ClosedPublic

Authored by quantum on May 21 2019, 10:47 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

quantum created this revision.May 21 2019, 10:47 AM
tlively added inline comments.May 21 2019, 12:43 PM
llvm/test/CodeGen/WebAssembly/return-address.ll
6 ↗(On Diff #200541)

Can you add a comment to this test saying what it is for? Also, it would be good to add a test to check for an error on wasm32-unknown-unknown.

11 ↗(On Diff #200541)

how about naming this test_returnaddress to call attention to what's being tested.

quantum updated this revision to Diff 200575.May 21 2019, 12:56 PM
  • Renamed test function in LLVM IR to test_returnaddress.
  • Created test for wasm32-unknown-unknown and expect it to fail.

Would it make sense to take this opportunity to rename the library function, from emscripten_return_address to something more general, like __wasm_return_address or __wasm_callsite_identifier or something, so that non-Emscripten environments could implement it too if they wished?

tlively added inline comments.May 21 2019, 1:01 PM
llvm/test/CodeGen/WebAssembly/return-address-emscripten.ll
6 ↗(On Diff #200575)

Please add a newline after the explanatory comment to visually separate it from the CHECK lines.

llvm/test/CodeGen/WebAssembly/return-address-unknown.ll
2 ↗(On Diff #200575)

It would be better to pipe the error message to FileCheck and Check for the exact message you are expecting, rather than to just expect the test as a whole to fail.

quantum updated this revision to Diff 200576.May 21 2019, 1:04 PM
  • Check the error message on wasm32-unknown-unknown
  • Added new lines to visually separate test description from checks

Would it make sense to take this opportunity to rename the library function, from emscripten_return_address to something more general, like __wasm_return_address or __wasm_callsite_identifier or something, so that non-Emscripten environments could implement it too if they wished?

I would image that WASI would want its own WASI-namespaced version of this. Are other system functions going to be general over all wasm OSes?

Would it make sense to take this opportunity to rename the library function, from emscripten_return_address to something more general, like __wasm_return_address or __wasm_callsite_identifier or something, so that non-Emscripten environments could implement it too if they wished?

I think we can always check the subtarget before calling setLibcallName to accommodate alternative names for this function, if other targets decide to implement return address with a library call.

It is not yet clear how non-Emscripten environments will implement __builtin_return_address at all. It would be premature to declare a function for them to implement.

tlively added inline comments.May 21 2019, 1:17 PM
llvm/test/CodeGen/WebAssembly/return-address-unknown.ll
2 ↗(On Diff #200576)

I don't think this XFAIL should be necessary any more, right?

quantum marked an inline comment as done.May 21 2019, 1:19 PM
quantum added inline comments.
llvm/test/CodeGen/WebAssembly/return-address-unknown.ll
2 ↗(On Diff #200576)

It is necessary. Without the XFAIL it gets count as an unexpected failure.

A function like this might be implemented in compiler_rt/libgcc or libc, which might in make future WASI system calls or future wasm language features underneath. An interface like __wasm_return_address doesn't commit to a particular implementation. I'm not suggesting enabling it for other targets now, just asking if it makes sense to pick a name which isn't tied to Emscripten.

If Emscripten wishes to support the WASI ABI in the future, it may have to support whatever name WASI ends up picking in addition to the Emscripten-specific name. However, it isn't a show stopper if Emscripten ends up having to support multiple names, so I don't have a strong opinion here.

Nice!

  • Please run clang-format before you submit patches
  • When you did what the review suggested, please click 'Done' checkbox, so that the reviewer will know which one was taken care of and which one was not.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
994 ↗(On Diff #200576)

Does this mean we ignore the whole node if the argument is not a constant? How about signaling a failure?

llvm/lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp
468 ↗(On Diff #200576)

How about moving this up, before all undefined ones?

496 ↗(On Diff #200576)

This looks irrelevant to the comment about floats above. Maybe add a newline and comment for this?

quantum updated this revision to Diff 200588.May 21 2019, 2:35 PM
quantum marked 6 inline comments as done.

Performed requested changes.

quantum marked an inline comment as done.May 21 2019, 2:35 PM
quantum added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
994 ↗(On Diff #200576)

verifyReturnAddressArgumentIsConstant takes care of generating the error. This code is copied from what other targets are doing.

quantum updated this revision to Diff 200589.May 21 2019, 2:40 PM

Used clang-format.

sbc100 added inline comments.May 21 2019, 2:59 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
277 ↗(On Diff #200589)

I kind of agree with Dan re: the naming of this.

If anything we should we at least put "__" at the start so that its clear this is not a normal user function.

What is this called on other plaforms? Why not just call it "__builtin_return_address"?

992 ↗(On Diff #200589)

Should we just lower this anyway? And thereby turn this from compile error into a link error if you happen to have not implemented it?

quantum marked 4 inline comments as done.May 21 2019, 4:14 PM
quantum added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
277 ↗(On Diff #200589)

The function already exists in emscripten under that name. In emscripten, these functions don't have the __ prefix, for example, emscripten_get_callstack.

On other platforms, __builtin_return_address is lowered to code that reads LR or pulls it out of the stack, instead of a helper function.

992 ↗(On Diff #200589)

I don't think so. We aren't sure what this can be lowered to on other WASM targets. Emscripten is the first target to use a library function to compute return addresses, and I would hesitate to extend this to other targets.

tlively added inline comments.May 21 2019, 5:40 PM
llvm/test/CodeGen/WebAssembly/return-address-unknown.ll
2 ↗(On Diff #200576)

Oh, because llc is still "failing", even though the test now passes. Try adding a not to the run line, like ; RUN: not llc..

quantum updated this revision to Diff 200619.May 21 2019, 5:58 PM
quantum marked 2 inline comments as done.

Use not instead of XFAIL

quantum marked an inline comment as done.May 21 2019, 5:59 PM
tlively accepted this revision.May 21 2019, 6:18 PM

If a cross-platform version of this function emerges, we can always update the name here and in emscripten later. I think this is fine to merge now because it is consistent in the context of things that exist today and can easily be extended. I also think it is good to leave this as a compiler error; there's not much of a difference between this and any other runtime function that wasm does not support but that an individual user might want to supply. Maybe a separate PR should make all runtime functions linker errors instead of compile errors?

This revision is now accepted and ready to land.May 21 2019, 6:18 PM

I'll wait for a second LGTM and take care of committing this tomorrow.

sbc100 added inline comments.May 22 2019, 2:07 AM
llvm/include/llvm/IR/RuntimeLibcalls.def
543 ↗(On Diff #200619)

This seems a little strange given that its in generic code and also its that only actual LIBCALL with nullptr as the second arg here. Can you explain why this is needed? I would imagine it should have a comment since its different from all the others.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
277 ↗(On Diff #200589)

OK, fair enough.

Can you add a TODO() here to replace this with a more generic thing (wither from tool-conventions, or from wasi I guess) in the future?

llvm/lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp
497 ↗(On Diff #200619)

Does this need to be protected by an if (is_emscripten()) check too?

Actually what happens right now if you *don't* land this? Will it result in link error: undefined symbol __builtin_return_address?

Is so, why not just make __builtin_return_address and alias for emscripten_return_address in emscripten and avoid this change completely?

quantum marked 4 inline comments as done.May 22 2019, 9:56 AM

Actually what happens right now if you *don't* land this? Will it result in link error: undefined symbol __builtin_return_address?

Is so, why not just make __builtin_return_address and alias for emscripten_return_address in emscripten and avoid this change completely?

What happens right now is llc just fails to build with the error message

WebAssembly hasn't implemented __builtin_return_address

This message was removed in this diff and replaced with a concrete implementation for Emscripten.

__builtin_return_address actually a compiler intrinsic and not a real function. I would have just implemented it in emscripten if it was a function.

llvm/include/llvm/IR/RuntimeLibcalls.def
543 ↗(On Diff #200619)

This is in generic code because it appears that all library calls must be defined here. There is some common mechanism for TargetLowering to lower things to library calls that handles declaring external symbols and other stuff.

As for nullptr, this is not the first one to use it. This appears to be the name used for functions that don't have a standardized name to use as default, and putting the emscripten name would not have made sense here.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
277 ↗(On Diff #200589)

Sure I'll add a note here.

llvm/lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp
497 ↗(On Diff #200619)

I don't think this is necessary. It won't be trying to look up the emscripten name in the map if we aren't on emscripten, so it doesn't hurt to have it in the map. The other direction is governed by a separate map.

quantum updated this revision to Diff 200787.May 22 2019, 10:34 AM
quantum marked 2 inline comments as done.

Add a TODO about future action regarding emscripten_return_address libcall, depending on what other WASM backends end up doing.

quantum marked 2 inline comments as done.May 22 2019, 5:46 PM
This revision was automatically updated to reflect the committed changes.