This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add functions for EHScopes
ClosedPublic

Authored by aheejin on May 17 2018, 1:17 AM.

Details

Summary

There are functions using the term 'funclet' to refer to both

  1. an EH scopes, the structure of BBs that starts with catchpad/cleanuppad and ends with catchret/cleanupret, and
  2. a small function that gets outlined in AsmPrinter, which is the original meaning of 'funclet'.

So far the two have been the same thing; EH scopes are always outlined
in AsmPrinter as funclets at the end of the compilation pipeline. But
now wasm also uses scope-based EH but does not outline those, so we now
need to correctly distinguish those two use cases in functions.

This patch splits MachineBasicBlock::isFuncletEntry into
isFuncletEntry and isEHScopeEntry, and
MachineFunction::hasFunclets into hasFunclets and hasEHScopes, in
order to distinguish the two different use cases. And this also changes
some uses of the term 'funclet' to 'scope' in getFuncletMembership and
change the function name to getEHScopeMembership because this function
is not about outlined funclets but about EH scope memberships.

This change is in the same vein as D45559 (rL332667).

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.May 17 2018, 1:17 AM
aheejin added a comment.EditedMay 17 2018, 1:41 AM

I don't really know about SEH, but it looks like cleanup pads are treated as funclet entries in all personalities, whereas catchpads should not be treated as funclet entries in SEH, right? So I tried to preserve that semantics, but one thing I don't understand is why we have to set isEHFuncletEntry in findUnwindDestinations again if we set them in visitCatchPad and visitCleanupPad. Anyway that's what the current code is doing and I tried to preserve the same behavior. I ran all existing tests including SEH ones and all of them passed (except XFAILs).

aheejin retitled this revision from [WebAssembly] Add isEHScopeEntry / setIsEHScopeEntry to [WebAssembly] Add MachineBasicBlock::isEHScopeEntry & setIsEHScopeEntry.
dschuff added inline comments.May 17 2018, 1:02 PM
include/llvm/CodeGen/MachineBasicBlock.h
387 ↗(On Diff #147264)

Is this comment the right place to note the difference between and EH scope and EH funclet in the IR? Is an EH funclet just a scope that will become outlined?

aheejin added inline comments.May 17 2018, 1:48 PM
include/llvm/CodeGen/MachineBasicBlock.h
387 ↗(On Diff #147264)

Not sure what you mean..? Yes, a funclet is a scope that will be outlined. So these two are not exclusive; isEHFuncletEntry will be a subset of isEHScopeEntry. Do you mean I should elaborate more on the differences between the two here?

dschuff added inline comments.May 17 2018, 2:46 PM
include/llvm/CodeGen/MachineBasicBlock.h
387 ↗(On Diff #147264)

Yeah, somewhere there should be a comment that says what the difference between an EH scope and an EH funclet, basically just what you said in your reply. I don't know if this is the place, but somewhere. It should also go in the commit message.

lib/CodeGen/Analysis.cpp
679 ↗(On Diff #147264)

If this code only cares about whether a block is in a scope (as opposed to a funclet), should the variable names and/or comments here be updated too?

This revision is now accepted and ready to land.May 17 2018, 4:07 PM
aheejin updated this revision to Diff 147474.May 18 2018, 4:57 AM
aheejin marked 3 inline comments as done.
  • Address comments
aheejin updated this revision to Diff 147475.May 18 2018, 4:59 AM
  • funclets -> scopes in comments / strings
Harbormaster completed remote builds in B18317: Diff 147475.
aheejin updated this revision to Diff 147477.May 18 2018, 5:00 AM
  • collectScopeMembers -> collectEHScopeMembers
aheejin updated this revision to Diff 147479.May 18 2018, 5:02 AM
  • getScopeMembership -> getEHScopeMembership
aheejin added inline comments.May 18 2018, 5:04 AM
lib/CodeGen/Analysis.cpp
679 ↗(On Diff #147264)

Done. Ended up renaming a few methods and adding MachineFunction::hasEHScopes and MachineFunction::setHasEHScopes.
This function has been renamed as not getScopeMembership but getEHScopeMembership because this function is being used in many places and 'scope' only sounds little too general and ambiguous.

aheejin updated this revision to Diff 147579.May 18 2018, 1:43 PM

comment fix

aheejin retitled this revision from [WebAssembly] Add MachineBasicBlock::isEHScopeEntry & setIsEHScopeEntry to [WebAssembly] Add functions for EHScopes.May 18 2018, 1:45 PM
aheejin edited the summary of this revision. (Show Details)
aheejin edited the summary of this revision. (Show Details)
aheejin edited the summary of this revision. (Show Details)
aheejin edited the summary of this revision. (Show Details)
aheejin updated this revision to Diff 147698.May 19 2018, 9:17 PM
  • clang-format
This revision was automatically updated to reflect the committed changes.