Page MenuHomePhabricator

[WebAssembly] Support instruction selection for catching exceptions
ClosedPublic

Authored by aheejin on Mar 5 2018, 6:27 AM.

Details

Summary

This lowers exception catching-related instructions:

  1. Lowers wasm.catch intrinsic to catch instruction
  2. Removes catchpad and cleanuppad instructions; they are not necessary after isel phase (MachineBasicBlock::isEHFuncletEntry() or MachineBasicBlock::isEHPad() can be used instead.)
  3. Lowers catchret and cleanupret instructions to pseudo catchret and cleanupret instructions in isel, which will be replaced with other instructions in WebAssemblyExceptionPrepare pass.
  4. Adds WebAssemblyExceptionPrepare pass, which replaces catchret/cleanupret instructions with br and rethrow instructions appropriately.

Currently this does not handle lowering of intrinsics related to LSDA info generation (wasm.landingpad.index and wasm.lsda), because they cannot be tested without implementing EHStreamer's wasm-specific handlers. They are marked as TODO, which is needed to make isel pass. Also this does not generate try and end_try markers yet, which will be handled in later patches.

This patch is based on the first wasm EH proposal.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Mar 5 2018, 6:27 AM
aheejin edited the summary of this revision. (Show Details)Mar 5 2018, 4:12 PM
aheejin planned changes to this revision.Mar 8 2018, 5:57 PM

If we use Windows EH instruction, this may have to change.

aheejin updated this revision to Diff 142556.Apr 15 2018, 4:24 AM

Changed the patch to use previous EH proposal and WinEH instructions

aheejin edited the summary of this revision. (Show Details)Apr 15 2018, 4:27 AM
aheejin added a reviewer: majnemer.
aheejin edited the summary of this revision. (Show Details)Apr 15 2018, 4:33 AM
aheejin added inline comments.Apr 15 2018, 4:45 AM
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
165 ↗(On Diff #142556)

Now EH labels can be present and they are position instructions. I think we can safely ignore them here and don't need to crash.

lib/Target/WebAssembly/WebAssemblyRegisterInfo.h
48 ↗(On Diff #142556)

This was needed to prevent assertions from these functions.

aheejin updated this revision to Diff 143315.Apr 20 2018, 7:42 AM
  • Add a missing condition
aheejin updated this revision to Diff 143316.Apr 20 2018, 7:44 AM

Diff was messed up in the last commit; fixed.

aheejin updated this revision to Diff 143390.Apr 20 2018, 2:25 PM
  • Deleted the assertion added in D45711 (rL330217) because now we can have BBs without terminators.
aheejin updated this revision to Diff 144851.May 2 2018, 4:29 AM
  • tweak test case
aheejin updated this revision to Diff 145597.May 7 2018, 5:09 PM

replaceFuncletReturnInstructions -> replaceFuncletReturns

aheejin updated this revision to Diff 145608.May 7 2018, 5:44 PM
  • clang-tidy
aheejin updated this revision to Diff 146087.May 10 2018, 1:18 AM
  • ExceptionPrepare -> ReplaceFuncletReturns
aheejin edited the summary of this revision. (Show Details)May 10 2018, 1:19 AM
aheejin edited the summary of this revision. (Show Details)
dschuff added inline comments.May 10 2018, 5:13 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1367 ↗(On Diff #146087)

Based on this comment I assume we are marking 'IsEHFuncletEntry' for a different purpose than MSVC (since we don't need a function prologue)? In that case I guess this comment should be updated. Likewise below. (what do we use it for?)

aheejin added inline comments.May 10 2018, 6:40 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1367 ↗(On Diff #146087)

Here the funclet means "BB structure starting from catchpad/cleanuppad and ending with catchret/cleanupret".

While I'm not very sure what that comment really means, I guess it's related to this code. I'm not very familiar with PrologEpilogInserter, but I guess that's the part wasm does not use, because we don't need to do caller-save and calle-save things.

OTOH, MachineBasicBlock::isEHFuncletEntry() method is used in several other places, and wasm needs at least a few of them, in which the method is used with the same meaning as isEHPad(). For example, here in getFuncletMembership, the method that computes funclet membership in order to problematic merges between funclets.

I think I should update the comment, but not very sure how. So apparently that method is used in PrologEpilogInserter but not only there and even wasm needs that at some point.

aheejin marked an inline comment as done.May 11 2018, 4:11 PM
aheejin added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1368 ↗(On Diff #146418)

@majnemer MachineBasicBlock::isEHFuncletEntry() is apparently used in a lot of places including prolog/epilog generaton, and because we don't need prolog/epilog on funclet boundaries, I think should not set this flag for wasm, but here in getFuncletMembership checks isEHFuncletEntry and wasm needs this part. So I tried to change this to isEHPad() assuming that in WinEH isEHFuncletEntry and isEHPad are the same thing, but apparently I was wrong because now all the SEH testse are failing, which I don't know much about.

I guess we are facing the same problem as in D45559: we have two different functions for the use cases of 'funclet':

  1. the BB structure starting from catchpad/cleanuppad and ending with catchret/cleanupret
  2. the real outlined functions

In WinEH those two were the same thing but in wasm we don't need to outline them, which is the problem. Thoughts?

aheejin updated this revision to Diff 146465.May 12 2018, 1:08 AM
  • tweak test case
aheejin updated this revision to Diff 146466.May 12 2018, 1:37 AM
  • Early exit if a function does not have a personality function
majnemer added inline comments.May 16 2018, 3:27 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1371–1374 ↗(On Diff #146466)

I think this would be cleaner as:

if (!IsWasmCXX)
  DAG.setRoot(DAG.getNode(ISD::CATCHPAD, getCurSDLoc(), MVT::Other, getControlRoot()));
6181 ↗(On Diff #146466)

Maybe make fill in the TODO with a rough sketch of what it needs to do?

aheejin updated this revision to Diff 147247.May 16 2018, 11:44 PM
aheejin marked 2 inline comments as done.

Address comments

aheejin updated this revision to Diff 147270.May 17 2018, 1:55 AM

rebased onto D47005

aheejin updated this revision to Diff 147271.May 17 2018, 1:56 AM

fix sorry

I split MachineBasicBlock::isEHFuncletEntry and MachineBasicBlock::setIsEHFuncletEntry into two functions in D47005, as in the same vein with D45559, and rebased this CL onto it. PTAL.

aheejin updated this revision to Diff 147699.May 19 2018, 9:30 PM
  • clang-format
aheejin updated this revision to Diff 148125.May 22 2018, 4:07 PM
  • Make wasm.get.exception/ehselector intrinsic take a token argument
aheejin updated this revision to Diff 148582.May 25 2018, 4:33 AM

Deleted ReplaceFuncletReturns pass and move the routine to ExceptionPrepare.

dschuff accepted this revision.May 29 2018, 4:46 PM

Otherwise LGTM

lib/Target/WebAssembly/WebAssemblyRegisterInfo.h
48 ↗(On Diff #142556)

There should be a comment about that here, since the mask is something that shouldn't apply to wasm.

This revision is now accepted and ready to land.May 29 2018, 4:46 PM
aheejin updated this revision to Diff 149165.May 30 2018, 11:03 AM
aheejin marked an inline comment as done.
  • Add a comment
aheejin edited the summary of this revision. (Show Details)May 31 2018, 3:28 PM
This revision was automatically updated to reflect the committed changes.