[PATCH] D62210: [WebAssembly] Implement __builtin_return_address for emscripten

[Guanzhong Chen via Phabricator via llvm-commits]

quantum marked 4 inline comments as done.
quantum added a comment.

In D62210#1511588 <https://reviews.llvm.org/D62210#1511588>, @sbc100 wrote:

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



================
Comment at: llvm/include/llvm/IR/RuntimeLibcalls.def:543
+// Return address
+HANDLE_LIBCALL(RETURN_ADDRESS, nullptr)
+
----------------
sbc100 wrote:
> 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.
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.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:277
+  // Define the emscripten name for return address helper.
+  setLibcallName(RTLIB::RETURN_ADDRESS, "emscripten_return_address");
+
----------------
sbc100 wrote:
> quantum wrote:
> > sbc100 wrote:
> > > 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"?
> > 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.
> 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?
Sure I'll add a note here.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp:497
+
+    Map["emscripten_return_address"] = RTLIB::RETURN_ADDRESS;
   }
----------------
sbc100 wrote:
> Does this need to be protected by an `if (is_emscripten())` check too?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62210/new/

https://reviews.llvm.org/D62210