This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add Wasm exception handling prepare pass
ClosedPublic

Authored by aheejin on Feb 25 2018, 9:51 AM.

Details

Summary

This adds a pass that transforms a program to be prepared for Wasm
exception handling. This patch is based on the previous wasm EH proposal
and makes use of Windows EH instructions.

Diff Detail

Event Timeline

aheejin created this revision.Feb 25 2018, 9:51 AM
majnemer added inline comments.
include/llvm/CodeGen/Passes.h
328

clang-format this

include/llvm/IR/IntrinsicsWebAssembly.td
42–61

Please add some white space before each comment block, it is tough to read this.

lib/CodeGen/WasmEHPrepare.cpp
181–184

No need to brace.

289–292

What if this landing pad is dominated by two landing pads which are both top-level?

aheejin updated this revision to Diff 135848.Feb 25 2018, 8:21 PM
aheejin marked 3 inline comments as done.
  • Address comments
aheejin added inline comments.Feb 25 2018, 8:29 PM
lib/CodeGen/WasmEHPrepare.cpp
289–292

It's OK because the LSDA address is the same throughout a function. Actually we can store it just once in the entry block and that would be sufficient, but that would incur additional cost even when no exception occurs, so I didn't do that.

aheejin updated this revision to Diff 136888.EditedMar 2 2018, 5:08 PM
  • rebase
aheejin updated this revision to Diff 136956.Mar 5 2018, 1:01 AM
  • comment line wrapping fix
dschuff added inline comments.Mar 7 2018, 4:01 PM
include/llvm/IR/IntrinsicsWebAssembly.td
43

This description is a little confusing. Maybe it should start with a high-level description of its semantics in terms of LLVM IR (i.e. without reference to the wasm primitives that implement it), and then after that put this description of how it's lowered.

lib/CodeGen/DwarfEHPrepare.cpp
209 ↗(On Diff #136956)

This can go inside the pruneUnreachableresumes conditional since it's only used there.

lib/CodeGen/TargetPassConfig.cpp
662

this could be addPass(createDwarfEHPass(/*PruneUnreachableResumes=*/false) to make it more greppable.

lib/CodeGen/WasmEHPrepare.cpp
17

I assume the actual value of the index is a literal in the generated IR, and is assigned by this pass?

38

why not just delete it here from LLVM IR now that it's dead? or does it also get lowered somehow?

55

->represent a thrown exception

108

maybe put a TODO here to make a way to model the exception_ref in C++ so we can move more of the generated code into libraries.

196

Could these getDeclarations, getOrInsertFunctions, GlobalVariable fields etc be moved to doInitialization? Or maybe all except the one that requires the TargetMachine?

275

Could just use cast instead of dyn_cast and use its builtin assert.

286

could be /*isVolatile=*/true for readability. For that matter, why volatile?

aheejin planned changes to this revision.Mar 9 2018, 1:26 PM

Will defer landing this after we sort out how WinEH might affect this.

aheejin updated this revision to Diff 141623.Apr 9 2018, 5:13 AM
aheejin marked 4 inline comments as done.
  • Rebased onto master
  • Set personality function call as nothrow
  • Address previous comments
include/llvm/IR/IntrinsicsWebAssembly.td
43

This part will go away when we switch to windows EH instructions.

lib/CodeGen/DwarfEHPrepare.cpp
209 ↗(On Diff #136956)

After we switch to windows EH, we don't need this anymore.

lib/CodeGen/TargetPassConfig.cpp
662

After we switch to windows EH, we don't need this anymore.

lib/CodeGen/WasmEHPrepare.cpp
17

Yes, it is generated by this pass, assigning an integer from 0 when traversing eh pads.

38

Landingpad instructions are lowered in [[ https://github.com/llvm-mirror/llvm/blob/b273503311ade5f98f2879195ed9c5655c3fff1b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L2468-L2517 | SelectionDAG::visitLandingPad ]], so we needed that by then. But anyway, after we switch to winEH we are not gonna have landingpads anymore; instead we are gonna have catchpads and cleanuppads.

55

Done.. but this part is gonna change anyway.

108

If we switch to winEH, we don't need it for now; will do it once we get to use except_ref later.

196

Done. I initially put them in runOnFunction because I didn't want to insert them if the program does not use exceptions thus has no landing pad. But they are gonna be deleted anyway at the end if unused, so I guess it's fine to move.

286

I didn't want to make them optimized away, so I thought setting isVolatile to true would be safer, no?

aheejin planned changes to this revision.EditedApr 9 2018, 5:14 AM

This will change significantly due to switching to winEH instructions, which hasn't been applied yet. The previous patch just addresses existing comments.

aheejin updated this revision to Diff 141624.Apr 9 2018, 5:17 AM
  • Deleted debugging printf lines that was uploaded by mistake
aheejin planned changes to this revision.Apr 9 2018, 5:18 AM
aheejin updated this revision to Diff 141985.EditedApr 11 2018, 5:47 AM
  • Now Wasm EH uses WinEH instructions (catchswitch, catchpad/ret and cleanupad/ret) - Wasm EH runs WinEHPrepare before WasmEHPrepare, with a flag that it only demotes PHIs in catchswitch blocks.
  • Go back to the previous Wasm EH proposal
  • Moved a call to wasm.landingpad.index() intrinsic from invoke blocks to EH pad blocks, which will be more intuitive
  • Add DemoteCatchSwitchOnly option to WinEHPrepare for Wasm EH
  • Some cosmetic changes
  • Rebase
aheejin updated this revision to Diff 141986.Apr 11 2018, 5:49 AM

I messed up diff in last commit; fixing that

aheejin edited the summary of this revision. (Show Details)
aheejin removed subscribers: javed.absar, gbedwell.
aheejin updated this revision to Diff 141987.Apr 11 2018, 5:56 AM
  • Cosmetic change
aheejin updated this revision to Diff 141988.Apr 11 2018, 5:56 AM

Messed up diff again..

Harbormaster completed remote builds in B16970: Diff 141988.
aheejin updated this revision to Diff 142134.Apr 12 2018, 2:13 AM
  • Extract personality function stuff to D45559
  • Rebase
aheejin updated this revision to Diff 145604.May 7 2018, 5:33 PM
  • clang-tidy
dschuff added inline comments.May 10 2018, 4:33 PM
lib/CodeGen/WasmEHPrepare.cpp
64

probably worth making this "foreign exceptions (e.g. JavaScript)" since that's the expected case.

70

s/"stack"/"the stack" in most of the uses in this comment block.

218

could be else if

299

I still don't see why these stores need to be volatile. They store to a global so they shouldn't be touched before linking, and when the optimizer can see the uses, then it should do the right thing anyway?

lib/CodeGen/WinEHPrepare.cpp
54

Why do catchswitch phis need to be removed if we don't outline any funclets?

aheejin updated this revision to Diff 146262.May 10 2018, 5:26 PM
aheejin marked 4 inline comments as done.
  • Address comments
lib/CodeGen/WinEHPrepare.cpp
54

Because catchswitch BBs are removed, or more precisely, not copied to MachineBasicBlock in ISel.

aheejin added inline comments.May 10 2018, 6:13 PM
lib/CodeGen/WinEHPrepare.cpp
54

The code that skips catchswitch blocks is here.

The Wasm code looks good with the one comment so LGTM if @majnemer is OK with the modification to WinEHPrepare.

lib/CodeGen/WasmEHPrepare.cpp
196

Oh, that's a good point; I think you had it right the first time and that it does make sense not to unnecessarily add the globals (even if the cost is a bit of redundancy). Can you put it back? (sorry!)

aheejin updated this revision to Diff 147405.May 17 2018, 3:45 PM
  • Move stuff out of doInitialization again
aheejin marked an inline comment as done.May 17 2018, 3:48 PM
majnemer added inline comments.May 17 2018, 4:36 PM
lib/CodeGen/WasmEHPrepare.cpp
186–188

I'd save and reuse BB.getFirstNonPHI.

270–276

What if BB branches to a block which calls wasm.get.exception? What if two different catchpads branch to a block which calls wasm.get.exception? ISTM that you might want to give it the token of the catchpad/cleanuppad somehow?

aheejin updated this revision to Diff 147458.May 18 2018, 3:05 AM
aheejin marked an inline comment as done.
  • Address comments
aheejin added inline comments.May 18 2018, 4:12 AM
lib/CodeGen/WasmEHPrepare.cpp
270–276

Oh, I haven't thought about that possibility.. I kind of assumed that wasm.get.exception and wasm.get.selector intrinsics cannot be reordered past a catchpad's terminator because [[ https://github.com/llvm-mirror/llvm/blob/0ec6e3688b87112c68c882be89e59a7f6c1a0382/include/llvm/IR/IntrinsicsWebAssembly.td#L45-L47 | they are marked as IntrHasSideEffects ]]. Is this assumption wrong?

And can I attach a funclet bundle to function calls that are marked as nounwind? Looks like CodeGenFunction::getBundlesForFunclet function does not add a funclet bundle operand in case the callee cannot throw. I guess I still manually can add a funclet operand bundle though. Did you mean that?

majnemer added inline comments.May 18 2018, 8:45 AM
lib/CodeGen/WasmEHPrepare.cpp
270–276

I don't think they will get reordered past a catchret. However, I don't see anything that prevents:

bb0:
  catchpad []
  br cont

cont:
  wasm.get.exception

or

bb0:
  catchpad []
  br common

bb1:
  catchpad []
  br common

common:
  wasm.get.exception

Instead of attaching a funclet bundle to the intrinsic call, I'd do what the int_coro_* family of intrinsics do and pass the token as an argument to wasm.get.exception/wasm.get.selector.

Then, it is as easy as walking all the users of the catchpad token to find which wasm.get.exception/wasm.get.selector you need to replace.

aheejin updated this revision to Diff 147697.May 19 2018, 9:14 PM
aheejin marked an inline comment as done.
  • Make wasm.get.exception/selector intrinsics take a token argument
lib/CodeGen/WasmEHPrepare.cpp
270–276

Thank you! That's a good point I have't thought about. I updated D44931 accordingly as well.

dschuff accepted this revision.May 29 2018, 4:34 PM
This revision is now accepted and ready to land.May 29 2018, 4:34 PM
aheejin updated this revision to Diff 149024.May 29 2018, 7:44 PM
  • Set personality function call wrapper as nothrow
aheejin added a comment.EditedMay 31 2018, 12:43 PM

@dschuff Are you OK with my latest change?

Yes, still LGTM

This revision was automatically updated to reflect the committed changes.