[PATCH] D45559: [WebAssembly] Add Wasm personality and usesWindowsEHInstructions()

[Heejin Ahn via Phabricator via llvm-commits]

aheejin marked an inline comment as done.
aheejin added a comment.

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 `beginFunclet` and `endFunclet` handlers in `AsmPrinterHandler` <https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/AsmPrinterHandler.h#L61-L64> if they want outline them. Windows implements them <https://github.com/llvm-mirror/llvm/blob/4bd3e6c6db43dc9b27a6fb85b3936e516afd8f64/lib/CodeGen/AsmPrinter/WinException.cpp#L182-L272>, 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.

- `catchpad` and `cleanuppad` instructions are both categorized as 'funcletpad instructions', and the class `FuncletPadInst` <https://github.com/llvm-mirror/llvm/blob/f100a5dc31e462f95d93680313d8f166edde8509/include/llvm/IR/InstrTypes.h#L1153-L1212> is a parent of `CatchPadInst` <https://github.com/llvm-mirror/llvm/blob/f100a5dc31e462f95d93680313d8f166edde8509/include/llvm/IR/Instructions.h#L4225-L4273> and `CleanupPadInst` <https://github.com/llvm-mirror/llvm/blob/f100a5dc31e462f95d93680313d8f166edde8509/include/llvm/IR/Instructions.h#L4184-L4223>.
- Also in many contexts, 'funclet' is used to denote a BB structure that starts from `catchpad`/`cleanuppad` and ends with `catchret`/`cleanupret`. You can think of it as something like wasm's 'catch' part, with nesting structure.

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 <https://github.com/llvm-mirror/llvm/blob/3372396f7080f9ea0c335dab0441c65a39a048f1/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp#L100-L120> 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



================
Comment at: include/llvm/Analysis/EHPersonalities.h:30
   GNU_CXX_SjLj,
+  GNU_CXX_Wasm,
   GNU_ObjC,
----------------
dschuff wrote:
> 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?
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?


================
Comment at: lib/Analysis/MustExecute.cpp:55
     if (Constant *PersonalityFn = Fn->getPersonalityFn())
-      if (isFuncletEHPersonality(classifyEHPersonality(PersonalityFn)))
+      if (usesWindowsEHInstructions(classifyEHPersonality(PersonalityFn)))
         SafetyInfo->BlockColors = colorEHFunclets(*Fn);
----------------
dschuff wrote:
> do we actually need to run `colorEHFunclets` for wasm?
Yes, that is called [[ https://github.com/llvm-mirror/llvm/blob/91ba6e733866ca1c00afa0aee5a3016c38123587/lib/CodeGen/WinEHPrepare.cpp#L670 | 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`.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1572
     EHPersonality Personality = classifyEHPersonality(CallerPersonality);
-    if (isFuncletEHPersonality(Personality)) {
+    if (usesWindowsEHInstructions(Personality)) {
       Optional<OperandBundleUse> ParentFunclet =
----------------
dschuff wrote:
> this code is also funclet-related. have you checked that it does what we want?
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D45559