Page MenuHomePhabricator

[WebAssembly] Add WebAssemblyLateEHPrepare pass
ClosedPublic

Authored by aheejin on May 13 2018, 4:03 AM.

Details

Summary

Add WebAssemblyLateEHPrepare pass that does several small jobs for
exception handling. This runs before CFGSort, and is different from
WasmEHPrepare pass that runs before ISel, even though the names are
similar.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.May 13 2018, 4:03 AM
aheejin updated this revision to Diff 146507.May 13 2018, 4:37 AM

Removed a blank line

aheejin updated this revision to Diff 146846.May 15 2018, 8:46 AM

removed setIsEHFuncletEntry

aheejin updated this revision to Diff 146907.May 15 2018, 1:28 PM
  • Deleted successors of catch-all terminatepads
aheejin updated this revision to Diff 146915.May 15 2018, 2:01 PM
  • removed an unnecessary call from a test case
aheejin updated this revision to Diff 147017.May 16 2018, 2:12 AM
  • debug location fix

@majnemer Continuing the discussion from D44134:

So the background was:
__clang_call_terminate is called within a cleanuppad. But cleanuppad does not give you an exception object, which __clang_call_terminate takes as an argument. So even if it is a cleanuppad, we added a wasm catch instruction there to catch an exception object. But this cleanuppad should be entered even when we catch a foreign (e.g. JavaScript) exception, which we have no exception object to deal with. So in that case we just directly all std::terminate.

So, when the clang-generated code is like

terminate:
  %exn = catch(0)
  call void @__clang_call_terminate(%exn)
  unreachable

processTerminatePads() appends one more terminatepad right after this terminatepad, like:

terminate-catchall:
  catch_all
  call @std::terminate
  unreachable

So the intention was, if we catch a C++ exception we go to terminate, and if we catch a foreign exception we end up in terminate-catchall. Here I also assumed that the terminatepad is a single BB, but as you noted, it may not be. But because this pass runs even before ExceptinInfo (D44134), this does not have the exception grouping information here.

Do you think I can merge the terminatepad into a single BB in this pass, so this duplication of terminatepad process will be easier? I can't think of a situation in which a terminatepad can have complicated control flows other than a few straight-line BBs. (Actually I can't even imagine what kind of llvm pass is gonna divide a single terminate into two or more, but we can't guarantee that can't happen, so..)

Also, the other thing is, I merge terminate pads if there are more than two of them in mergeTerminatePads() here. This is in principle violation of EH scope membership, but this happens after all MachineFunctionPasses and only remaining wasm passes after this are CFGSort and CFGStackify, so I guess it should be OK.

aheejin updated this revision to Diff 148126.May 22 2018, 4:15 PM
  • Make wasm.get.exception/ehselector intrinsic take a token argument
aheejin updated this revision to Diff 148583.May 25 2018, 4:36 AM
  • Rebased onto D44090
  • Not this pass deletes the following unreachable instruction after adding a rethrow instruction.
aheejin planned changes to this revision.May 31 2018, 12:45 PM

Planning two changes:

  1. Handling of the terminate pad
  2. Removing unreachable BB after rethrow
aheejin updated this revision to Diff 151637.EditedJun 17 2018, 1:34 AM
  • Added a method to ensure a terminate pad is a single BB
  • Using WasmEHInfo, now this adds RETHROW or RETHROW_TO_CALLER instruction depending on its unwind destination
  • Ensured a rethrow instruction is in the same BB with __cxa_rethrow() call and deleted all unreachable children
  • In hoistCatches, added handling of a case when catch instruction is not in the EH pad BB anymore
  • Now uses several utility functions in WebAssemblyUtilities.cpp
  • Added helper functions
  • Misc. improvements here and there
aheejin updated this revision to Diff 151640.Jun 17 2018, 2:02 AM
  • Delete an unreachable after rethrow
dschuff added inline comments.Jun 19 2018, 2:03 PM
lib/Target/WebAssembly/WebAssemblyExceptionPrepare.cpp
27 ↗(On Diff #151640)

I didn't think about the confusing-ness of this name when we put it in originally; maybe we could call it 'WebAssemblyMachineEHPrepare' or 'WebAssemblyLateEHPrepare'

110 ↗(On Diff #151640)

So a function can rethrow but not have a personality function?

168 ↗(On Diff #151640)

'instruction is'

212 ↗(On Diff #151640)

Would it make more sense to just have the rethrow inside of cxa_rethrow (at least if the destination is the caller)? It might save some on code size? I guess we would just have to put an 'unreachable' after the call and it wouldn't matter; or is there some other reason?

aheejin updated this revision to Diff 151988.Jun 19 2018, 3:25 PM
aheejin marked 2 inline comments as done.
  • WebAssemblyExceptionPrepare -> WebAssemblyLateEHPrepare
  • Typo fix
aheejin added inline comments.Jun 19 2018, 3:25 PM
lib/Target/WebAssembly/WebAssemblyExceptionPrepare.cpp
27 ↗(On Diff #151640)

Hmm, not to be confused with WasmEHPrepare? Yeah, that makes sense.. I'll go with WebAssemblyLateEHPrepare because 'Machine' sounds like it has its same or similar counterpart in LLVM IR passes. (like, LoopInfo vs MachineLoopInfo)

110 ↗(On Diff #151640)

A function has its associated personality function when it has an EH pad (landingpad or catchpad/cleanuppad). Functions that call __cxa_rethrow() can rethrow to the caller, in which case the function doesn't have an EH pad. I didn't want to run all the functions below needlessly in that case.

212 ↗(On Diff #151640)

That is the spec question. The current implementation is based on the first proposal, in which rethrow can only be placed within a catch block. We can change this later if that is shown to save code size.

I'll do some experiments on that once I'm able to run things end-to-end, but I actually doubt if that would be saving much, because we can save one rethrow instruction if we put that within a function, but that also means we have to pass a first-class exception argument to __cxa_rethrow(), and to put that argument on the wasm stack we have to consume one more instruction anyway. And also, if we use first-class exception objects, now every catch clause has to have that if_except ~ else ~ end instruction sequence too, which also contributes to the code size.

Anyway, I'm not saying I'm against it though. I'm saying just the current implementation is based on the first proposal :$

dschuff accepted this revision.Jun 19 2018, 3:59 PM

OK, the pass looks good; don't know if @majnemer has any more feedback on this or D44134?

This revision is now accepted and ready to land.Jun 19 2018, 3:59 PM

@majnemer Do you think the problem we discussed in this CL and D44134 have been resolved?
The comment that started the discussion is here. So basically I tried to ensure a terminatepad is a single BB in this prepare pass.

aheejin retitled this revision from [WebAssembly] Add WebAssemblyExceptionPrepare pass to [WebAssembly] Add WebAssemblyLateEHPrepare pass.Jun 20 2018, 4:21 PM
aheejin edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.