This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add Wasm personality and isScopedEHPersonality()
ClosedPublic

Authored by aheejin on Apr 12 2018, 1:51 AM.

Details

Summary
  • Add wasm personality function
  • Re-categorize the existing isFuncletEHPersonality() function into two different functions: isFuncletEHPersonality() and isScopedEHPersonality(). This becomes necessary as wasm EH uses scoped EH instructions (catchswitch, catchpad/ret, and cleanuppad/ret) but not outlined funclets.
  • Changed some callsites of isFuncletEHPersonality() to isScopedEHPersonality() if they are related to scoped EH IR-level stuff.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Apr 12 2018, 1:51 AM
aheejin edited the summary of this revision. (Show Details)Apr 12 2018, 2:10 AM
dschuff added inline comments.May 7 2018, 5:31 PM
include/llvm/Analysis/EHPersonalities.h
30 ↗(On Diff #142129)

We might just want to call this 'Wasm', since it will be sort of a hybrid between MSVC (IR) and GNU (library API). Does the personality enum refer to just the personality function itself, or does it describe the scheme more generally?

80 ↗(On Diff #142129)

Generally in this change you've gone from a fairly generic "funclet-based" to a more specific "windows"-related terminology; but of course you aren't doing this for windows; maybe this could be usesCatchpadInstructions or usesCatchpadEH which would be shorter and still generic.

lib/Analysis/MustExecute.cpp
55 ↗(On Diff #142129)

do we actually need to run colorEHFunclets for wasm?

lib/CodeGen/DwarfEHPrepare.cpp
198 ↗(On Diff #142129)

Update the comment too (wasm isn't funclet-based)

lib/CodeGen/WinEHPrepare.cpp
109 ↗(On Diff #142129)

update comment.

lib/Transforms/Instrumentation/GCOVProfiling.cpp
553 ↗(On Diff #142129)

update comment

632 ↗(On Diff #142129)

comment

lib/Transforms/Utils/EscapeEnumerator.cpp
77 ↗(On Diff #142129)

error text

lib/Transforms/Utils/InlineFunction.cpp
1572 ↗(On Diff #142129)

this code is also funclet-related. have you checked that it does what we want?

aheejin edited the summary of this revision. (Show Details)May 8 2018, 2:20 AM
aheejin marked an inline comment as done.May 8 2018, 4:15 AM

Before addressing some of the comments, I think I need to clarify things.

Funclets are basically small functions, but they are not treated as separate functions in either LLVM IR or MachineInstr level. They get outlined in AsmPrinter, at the very end of the compilation process, by emitting prolog and epilog code directly in assembly or binary. Targets should implement [[ https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/AsmPrinterHandler.h#L61-L64 | beginFunclet and endFunclet handlers in AsmPrinterHandler ]] if they want outline them. Windows implements them, but in wasm we can leave them empty, because we don't really want that behavior.

But currently the term 'use funclets', both in function names and comments / error messages, is being used to mean both 'use windows EH IR' and 'use outlined funclets' in the code.

Currently 'use funclet' can mean either thing in LLVM. But in wasm we only want the IR level behavior but not the outlining behavior. An example of the latter, which deals with the real function outlining, is this code preparing information for LSDA generation for funclets, which we are not gonna use in wasm.

This is the reason why the term funclet is used everywhere, and it is not very clear how to differently name those two behaviors and whether I should fix all comments and error messages mentioning funclets. Can I assume the term 'funclet' only means the real outlined funclet and fix all of them?

cc @majnemer

include/llvm/Analysis/EHPersonalities.h
30 ↗(On Diff #142129)

I named it GNU_*** because the personality function itself is in GNU style, but this enum is used in more broad sense to check the scheme itself throughout the code. Should it be CXX_Wasm or Wasm then? Do you want to drop CXX part too?

lib/Analysis/MustExecute.cpp
55 ↗(On Diff #142129)

Yes, that is called here, and information created by this function is used in WinEHPrepare to clone BBs that belong to multiple funclets. (The cloning is in order to preserve strict scope structure and undo all problematic merges between funclets.)

This is not related to outlining behavior, but I'm not sure if I should change method names like this (there are more), because of the reason I mentioned in the top comment. They use the term 'funclet' here to mean the BB structure starting from catchpad/cleanuppad and ending with catchret/cleanupret.

lib/Transforms/Utils/InlineFunction.cpp
1572 ↗(On Diff #142129)

Yes. This code is about what to do when we inline functions that contains catchpad or cleanuppad instructions in LLVM IR. So it is not related to real funclet outlining behavior in AsmPrinter I mentioned in the top comment. If we use Windows EH IR, we need this.

uncheck an 'inline comment done' checkbox checked by mistake

As I said above, the term 'funclet' seems to be used pretty everywhere including clang. To summarize its usage,

  1. Small outlined functions themselves
  2. Funcletpad instructions, such as catchpad or cleanuppad
  3. The structure of BBs starting from catchpad/cleanuppad and ending with catchret/cleanupret, which can be nested
  4. This Windows IR -based EH itself

Now I'm thinking that maybe we should use isFuncletEHPersonality for the use cases here I replaced with usesWindowsEHInstructions, and come up with a new name to give to the use cases that pertain to real outlined small functions.

aheejin added a reviewer: rnk.May 9 2018, 1:50 PM
smeenai added a subscriber: smeenai.May 9 2018, 1:53 PM

As I said above, the term 'funclet' seems to be used pretty everywhere including clang. To summarize its usage,

  1. Small outlined functions themselves
  2. Funcletpad instructions, such as catchpad or cleanuppad
  3. The structure of BBs starting from catchpad/cleanuppad and ending with catchret/cleanupret, which can be nested
  4. This Windows IR -based EH itself

Now I'm thinking that maybe we should use isFuncletEHPersonality for the use cases here I replaced with usesWindowsEHInstructions, and come up with a new name to give to the use cases that pertain to real outlined small functions.

I think that is probably the most prudent. Really, "funclet" should be renamed to "scope" everywhere but the AsmPrinter, sorry about that...

But currently the term 'use funclets', both in function names and comments / error messages, is being used to mean both 'use windows EH IR' and 'use outlined funclets' in the code.
Currently 'use funclet' can mean either thing in LLVM. But in wasm we only want the IR level behavior but not the outlining behavior. An example of the latter, which deals with the real function outlining, is this code preparing information for LSDA generation for funclets, which we are not gonna use in wasm.

Right, I guess my point was that to me 'funclet' refers to just the outlined bits of code and therefore wouldn't apply to wasm.

Now I'm thinking that maybe we should use isFuncletEHPersonality for the use cases here I replaced with usesWindowsEHInstructions, and come up with a new name to give to the use cases that pertain to real outlined small functions.

I'm a little confused; this seems backwards to me. (I think that means that I agree with @majnemer that the stuff outside of AsmPrinter where the outlining happens should be renamed). That will hopefully not be too much yak-shaving. I do think it will result in much more understandable code than not doing it though.

Let me clarify. So there are the two options:

  1. Use 'funclet' to only mean outlined funclet in AsmPrinter

I guess @dschuff endorses this, right? I'm not sure what @majnemer meant. Actually this is what I first tried to do in this patch, but this seems to be a harder way now, because as I said, the term funclet is used to mean a lot of things and being used everywhere including both llvm and clang in all sorts of method names in comments, not only this personality functions.

  1. Leave the uses of 'funclet' as they are, delete usesWindowsEHInstructions function and change the uses of them back to isFuncletEHPersonality, and create something like usesFuncletLSDA to be used in the cases that only pertain to real outlined funclets. This seems to be less work, because we don't need to change the uses of 'funclet' everywhere...

Let me clarify. So there are the two options:

  1. Use 'funclet' to only mean outlined funclet in AsmPrinter

I guess @dschuff endorses this, right? I'm not sure what @majnemer meant. Actually this is what I first tried to do in this patch, but this seems to be a harder way now, because as I said, the term funclet is used to mean a lot of things and being used everywhere including both llvm and clang in all sorts of method names in comments, not only this personality functions.

  1. Leave the uses of 'funclet' as they are, delete usesWindowsEHInstructions function and change the uses of them back to isFuncletEHPersonality, and create something like usesFuncletLSDA to be used in the cases that only pertain to real outlined funclets. This seems to be less work, because we don't need to change the uses of 'funclet' everywhere...

OK, I think I didn't understand that the term 'funclet' is also used to describe the BB subgraph that will later be outlined (in addition to the blob after outlining). I think if we keep that definition for now (i agree with @majnemer that it's not ideal but let's suppose we don't want to audit every use of the term everywhere right now), then it makes sense to say that Wasm is a 'funclet EH personality' because it uses 'funcletpad' instructions, and then the current uses of isFuncletEHPersonality that aren't relevant to wasm (i.e. the uses that this current version of this CL doesn't touch) could be usesFuncletLSDA or even something directly "windows"-related (usesWindowsLSDA, isWIndowsEHPersonality, etc).

So yeah I guess that's option 2, and I think that's what @majnemer was saying is prudent for now. So we can do that, and then discuss later whether it makes sense to do a more comprehensive renaming.

aheejin added a comment.EditedMay 11 2018, 3:51 PM

Gentle ping @majnemer :) Do you think we can proceed with the option 2 @dschuff suggested?

majnemer added inline comments.May 16 2018, 3:24 PM
include/llvm/Analysis/EHPersonalities.h
65–92 ↗(On Diff #142129)

These two functions are _really_ confusing because the usesWindowsEHInstructions returns true for Wasm.

I think the function which returns true for wasm should be called isScopedEHPersonality.
The one which is only true for MSVC & CoreCLR should be called isFuncletEHPersonality

I think anything else really invites confusion.

aheejin updated this revision to Diff 147235.May 16 2018, 9:21 PM
aheejin marked 6 inline comments as done.
  • usesWindowsEHInstructions -> isScopedEHPersonality
aheejin added inline comments.May 16 2018, 9:21 PM
include/llvm/Analysis/EHPersonalities.h
30 ↗(On Diff #142129)

I renamed it to CXX_Wasm. Let me know if what you meant was to drop CXX part as well.

80 ↗(On Diff #142129)

Renamed it to isScopedEHPersonality, as @majnemer suggested.

I renamed usesWindowsEHInstructions to isScopedEHPersonality as @majnemer suggested. But there are still a lot of uses of the term 'funclet' to denote scopes in other function names, local variable names, and comments. I guess we should update all of them together sometime, but that can be a different CL.

aheejin retitled this revision from [WebAssembly] Add Wasm personality and usesWindowsEHInstructions() to [WebAssembly] Add Wasm personality and isScopedEHPersonality().May 16 2018, 9:26 PM
aheejin edited the summary of this revision. (Show Details)
majnemer accepted this revision.May 16 2018, 9:45 PM

LGTM with that final nit fixed.

include/llvm/Analysis/EHPersonalities.h
36 ↗(On Diff #147235)

I think Wasm_CXX is more in keeping with the style of the other enumerators.

This revision is now accepted and ready to land.May 16 2018, 9:45 PM
aheejin updated this revision to Diff 147239.May 16 2018, 9:52 PM

changed switch-case order. NFC

aheejin updated this revision to Diff 147240.May 16 2018, 9:53 PM

More switch-case order change; NFC

aheejin updated this revision to Diff 147241.May 16 2018, 9:59 PM
aheejin marked an inline comment as done.

CXX_Wasm -> Wasm_CXX

dschuff accepted this revision.May 17 2018, 9:14 AM

Thanks for your patience on this, I really do think this is a big readability improvement.

rnk added a comment.May 17 2018, 10:06 AM

Thanks for your patience on this, I really do think this is a big readability improvement.

I agree, thanks!

aheejin added a comment.EditedMay 17 2018, 1:35 PM

Thanks a lot for all your help!

This revision was automatically updated to reflect the committed changes.