Page MenuHomePhabricator

[WebAssembly] Make returns variadic
ClosedPublic

Authored by tlively on Oct 8 2019, 10:20 PM.

Details

Summary

This is necessary and sufficient to get simple cases of multiple
return working with multivalue enabled. More complex cases will
require block and loop signatures to be generalized to potentially be
type indices as well.

Diff Detail

Event Timeline

tlively created this revision.Oct 8 2019, 10:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2019, 10:20 PM
sbc100 added inline comments.Oct 9 2019, 11:32 AM
llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
1269

report_fatal_error so end users see this too?

Should there be a limit on how many returns we are willing to return in a multi? or should we have no fallback to sret at all?

dschuff added inline comments.Oct 9 2019, 1:28 PM
llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
1269

By this time we should have legalized everything, so this can be an assert because it should never happen?

tlively marked an inline comment as done.Oct 9 2019, 1:45 PM

Should there be a limit on how many returns we are willing to return in a multi? or should we have no fallback to sret at all?

My current thinking is that such a limit should be left up to the ABI logic in the frontend and that in the backend we should turn all aggregate returns by value into multivalue returns with no limit. WDYT?

llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
1269

@sbc100 is right that users should be able to see these errors. These asserts will be triggered when trying to emit nontrivial code with multivalue enabled.

aheejin accepted this revision.Oct 9 2019, 2:00 PM
aheejin added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
1309

Why <=2? If multivalues are supported in fastisel later, do we still have that limit?

This revision is now accepted and ready to land.Oct 9 2019, 2:00 PM
tlively updated this revision to Diff 224167.Oct 9 2019, 2:32 PM
  • Use report_fatal_error to report unimplemented functionality
tlively updated this revision to Diff 224170.Oct 9 2019, 2:37 PM
tlively marked an inline comment as done.
  • Bail instead of asserting in FastISel

I'll land this since it's just a WIP and not ready for users yet. I'm happy to discuss ABI limits and other considerations further and make changes in followups!

llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
1309

Oops, this comment should have been removed. I'm updating this to just bail out of FastISel in the case of multiple return for now. The proper condition to check is Ret->getNumOperands() > 1.

This revision was automatically updated to reflect the committed changes.