This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix conflict between ret legalization and sjlj
ClosedPublic

Authored by loladiro on Jul 30 2019, 11:12 AM.

Details

Summary

When the WebAssembly backend encounters a return type that doesn't
fit within i32, SelectionDAG performs sret demotion, adding an
additional argument to the start of the function that contains
a pointer to an sret buffer to use instead. However, this conflicts
with the emscripten sjlj lowering pass. There we translate calls like:

	call {i32, i32} @foo()

into (in pseudo-llvm

	%addr = @foo
	call {i32, i32} @__invoke_{i32,i32}(%addr)

i.e. we perform an indirect call through an extra function.
However, the sret transform now transforms this into
the equivalent of

%addr = @foo
%sret = alloca {i32, i32}
call {i32, i32} @__invoke_{i32,i32}(%sret, %addr)

(while simultaneously translation the implementation of @foo as well).
Unfortunately, this doesn't work out. The __invoke_ ABI expected
the function address to be the first argument, causing crashes.

There is several possible ways to fix this:

  1. Implementing the sret rewrite at the IR level as well and performing it as part of lowering to __invoke
  2. Fixing the wasm backend to recognize that __invoke has a special ABI
  3. A change to the binaryen/emscripten ABI to recognize this situation

This revision implements the middle option, teaching the backend to
treat __invoke_ functions specially in sret lowering. This is achieved
by

  1. Introducing a new CallingConv ID for invoke functions
  2. Adding a hook to CanLowerReturn to override which argument is used for the sret pointer.

This revision isn't quite complete and definitely needs a test
(and I'm considering dropping the hook in 2) in favor of just
swizzeling the arguments in lowerCall), but I have verified
that it indeed fixes the observed crashes, so it should be
sufficient to discuss whether this is the correct approach.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro created this revision.Jul 30 2019, 11:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 11:12 AM
aheejin added a subscriber: tlively.EditedJul 30 2019, 10:27 PM

Thank you for fixing this! This is a long-known bug we haven't fixed so far, because wasm C++ frontend lowers struct returns into sret arguments in the frontend, so we don't usually encounter this bug. Just out of curiosity, how did you discover this bug? (The reporter of this bug found it while using the Rust frontend, I heard)

I like this approach. I prefer the option 2 among the three options you described: 1 is redundantly doing what ISel is already doing, and 3 makes LLVM-backend generated code incorrect so we have to rely on the other tool to fix it, which seems not very desirable. As you said, having to add an extra argument to the interface of CanLowerReturn is a bit sad but I don't really mind that if others are OK with it.

@dschuff @tlively Any more opinions?

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
818 ↗(On Diff #212390)

Real nit: In this file it seems we mostly don't add braces to one-line ifs. The same for the if clause below.

I like this approach. I prefer the option 2 among the three options you described: 1 is redundantly doing what ISel is already doing, and 3 makes LLVM-backend generated code incorrect so we have to rely on the other tool to fix it, which seems not very desirable. As you said, having to add an extra argument to the interface of CanLowerReturn is a bit sad but I don't really mind that if others are OK with it.

@dschuff @tlively Any more opinions?

I agree that this is the best direction to go in. It avoids duplicated effort and magic.

loladiro updated this revision to Diff 213216.Aug 3 2019, 4:28 PM

Updated to add a test and confine the changes to the wasm backend code.

tlively accepted this revision.Aug 5 2019, 11:52 AM

LGTM with a small test format change.

llvm/test/CodeGen/WebAssembly/lower-em-sjlj-sret.ll
25 ↗(On Diff #213216)

You can pass -wasm-keep-registers to llc to keep the output in SSA form, which usually is more readable in tests.

This revision is now accepted and ready to land.Aug 5 2019, 11:52 AM
aheejin accepted this revision.Aug 5 2019, 2:13 PM

LGTM with nits (+ -wasm-keep-registers thing as @tlively said). Thank you!

llvm/test/CodeGen/WebAssembly/lower-em-exceptions.ll
19 ↗(On Diff #213216)

Nit: How about adding call cc{{.*}} here too show that invokes have been converted to calls, which is one of the main things this pass does? The same thing for similar lines in this file and lower-em-sjlj.ll too.

llvm/test/CodeGen/WebAssembly/lower-em-sjlj-sret.ll
13 ↗(On Diff #213216)

Real nit: we can maybe remove hidden

loladiro marked 2 inline comments as done.Aug 5 2019, 2:15 PM
loladiro added inline comments.
llvm/test/CodeGen/WebAssembly/lower-em-exceptions.ll
19 ↗(On Diff #213216)

Ok, will do prior to commit.

llvm/test/CodeGen/WebAssembly/lower-em-sjlj-sret.ll
13 ↗(On Diff #213216)

sure

This revision was automatically updated to reflect the committed changes.

Just out of curiosity, how did you discover this bug?

Realized I missed this question on my first read through. This came up compiling the julia system image. We do generally also lower things to sret. However, for things like Union{RegEx, Int64}, we emit that as a tagged union {jl_value_t *, i8}, which doesn't get sret'ed, because ideally we'd like it in two registers.

Oh I see. Thanks!