This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix wasm.lsda() optimization in WasmEHPrepare
ClosedPublic

Authored by aheejin on Apr 3 2020, 11:51 AM.

Details

Summary

When we insert a call to the personality function wrapper
(_Unwind_CallPersonality) for a catch pad, we store some necessary
info in __wasm_lpad_context struct and pass it. One of the info is the
LSDA address for the function. For this, we insert a call to
wasm.lsda(), which will be lowered down to the address of LSDA, and
store it in a field in __wasm_lpad_context.

There are exceptions to this personality call insertion: catchpads for
catch (...) and cleanuppads (for destructors) don't need personality
function calls, because we don't need to figure out whether the current
exception should be caught or not. (They always should.)

There was a little optimization to wasm.lsda() call insertion. Because
the LSDA address is the same throughout a function, we don't need to
insert a store of wasm.lsda() return value in every catchpad. For
example:

try {
  foo();
} catch (int) {
  // wasm.lsda() call and a store are inserted here, like, in
  // pseudocode,
  // %lsda = wasm.lsda();
  // store %lsda to a field in __wasm_lpad_context
  try {
    foo();
  } catch (int) {
    // We don't need to insert the wasm.lsda() and store again, because
    // to arrive here, we have already stored the LSDA address to
    // __wasm_lpad_context in the outer catch.
  }
}

So the previous algorithm checked if the current catch has a parent EH
pad, we didn't insert a call to wasm.lsda() and its store.

But this was incorrect, because what if the outer catch is catch (...)
or a cleanuppad?

try {
  foo();
} catch (...) {
  // wasm.lsda() call and a store are NOT inserted here
  try {
    foo();
  } catch (int) {
    // We need wasm.lsda() here!
  }
}

In this case we need to insert wasm.lsda() in the inner catchpad,
because the outer catchpad does not have one.

To minimize the number of inserted wasm.lsda() calls and stores, we
need a way to figure out whether we have encountered wasm.lsda() call
in any of EH pads that dominates the current EH pad. To figure that
out, we now visit EH pads in BFS order in the dominator tree so that we
visit parent BBs first before visiting its child BBs in the domtree.

We keep a set named ExecutedLSDA, which basically means "Do we have
wasm.lsda() either in the current EH pad or any of its parent EH
pads in the dominator tree?". This is to prevent scanning the domtree up
to the root in the worst case every time we examine an EH pad: each EH
pad only needs to examine its immediate parent EH pad.

  • If any of its parent EH pads in the domtree has wasm.lsda(), this means we don't need wasm.lsda() in the current EH pad. We also insert the current EH pad in ExecutedLSDA set.
  • If none of its parent EH pad has wasm.lsda()
    • If the current EH pad is a catch (...) or a cleanuppad, done.
    • If the current EH pad is neither a catch (...) nor a cleanuppad, add wasm.lsda() and the store in the current EH pad, and add the current EH pad to ExecutedLSDA set.

Diff Detail

Event Timeline

aheejin created this revision.Apr 3 2020, 11:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 11:51 AM
aheejin updated this revision to Diff 254872.Apr 3 2020, 11:52 AM
  • clang-format
aheejin edited the summary of this revision. (Show Details)Apr 3 2020, 11:54 AM
aheejin marked 2 inline comments as done.Apr 3 2020, 11:57 AM
aheejin added inline comments.
llvm/lib/CodeGen/WasmEHPrepare.cpp
307

This should've been NeedPersonality even before, which was a misnomer.

aheejin updated this revision to Diff 254878.Apr 3 2020, 11:59 AM
  • Comment fix
aheejin updated this revision to Diff 254885.Apr 3 2020, 12:24 PM
  • Add more comment
dschuff accepted this revision.Apr 3 2020, 12:56 PM

I'm totally in favor of verbose commit descriptions :D
You can probably just remove the first paragraph.

llvm/lib/CodeGen/WasmEHPrepare.cpp
345

In principle, passes should be thread-safe if they have separate LLVMContext objects. Can we hang init off the pass object instead?

This revision is now accepted and ready to land.Apr 3 2020, 12:56 PM
aheejin edited the summary of this revision. (Show Details)Apr 3 2020, 1:17 PM
aheejin edited the summary of this revision. (Show Details)
aheejin edited the summary of this revision. (Show Details)Apr 3 2020, 1:21 PM
aheejin updated this revision to Diff 254908.Apr 3 2020, 1:59 PM
  • Change the static variable init to a class member variable IsEHFunctionsSetup (Not sure whether I should AreEHFunctionsSetup instead, but I think usually variable names start with Is..? Let me know if this sounds weird.)
  • Move class member variable ExecutedLSDA to an auto variable in prepareEHPads. This variable is used only within that function.
aheejin marked an inline comment as done.Apr 3 2020, 2:00 PM

LGTM. I think IsFoo is probably fine even for plurals because it's a common convention as you say.
I guess if you wanted to be ultra-nitpicky-correct you could capitalize it as IsEHPadFunctionsSetUp (i.e. "set up" as 2 words instead of 1 since "setup" is a noun and "set up" is a verb phrase). But it's probably not worth bothering to change.

llvm/lib/CodeGen/WasmEHPrepare.cpp
128

Did you want to move this comment too?

aheejin updated this revision to Diff 255034.Apr 4 2020, 7:06 AM
aheejin marked an inline comment as done.

iIsEHPadFunctionsSetup -> IsEHPadFunctionsSetUp

llvm/lib/CodeGen/WasmEHPrepare.cpp
128

You mean the comment attached to ExecutedLSDA? No, now the long comment in prepareEHPads contains explanation of what that is for.

This revision was automatically updated to reflect the committed changes.