[PATCH] D15139: [IR] Reformulate LLVM's EH funclet IR
Joseph Tremoulet via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 2 15:03:02 PST 2015
JosephTremoulet added a comment.
Thanks for doing this, I think the new set of instructions are definitely cleaner. Feedback is mostly nits, the larger points are:
- The langref doesn't correctly describe the UB conditions around mismatched handler entries/exits.
- `isOutermostScope()` is either broken or poorly named vis-à-vis `getOuterScope()`.
- The utility of `catchswitch` producing a token that `catchpad` consumes seems dubious to me.
================
Comment at: docs/ExceptionHandling.rst:617
@@ -620,3 +616,3 @@
they must be the first non-phi instruction of a basic block that may be the
-unwind destination of an invoke: ``catchpad``, ``cleanuppad``, and
-``terminatepad``. As with landingpads, when entering a try scope, if the
+unwind destination of an invoke: ``catchswitch``, ``catchpad``, ``cleanuppad``,
+and ``terminatepad``. As with landingpads, when entering a try scope, if the
----------------
Maybe s/unwind destination of an invoke/destination of an EH flow edge/? Because `invoke`s can't go directly to `catchpad`s, but `catchpad`s are EH pads. Or s/unwind destination of an invoke/unwind destination of an invoke (or catchswitch or cleanupret)/...
================
Comment at: docs/ExceptionHandling.rst:697
@@ -711,1 +696,3 @@
+ lpad.catch: ; preds = %lpad.cleanup, %entry
+ %1 = catchswitch none, unwind label %lpad.terminate [label %catch.body]
----------------
I feel like having the parent token just plopped down after the operator like that reads awkwardly. I wonder what you would think about changing the syntax like so:
`catchswitch none, etc` -> `catchswitch etc`
`catchswitch %parent, etc` ->`catchswitch within %parent, etc`
(and similarly for cleanuppad/terminatepad)?
(related but not really relevant to this review, I sometimes wish I'd gone with @rnk's initial suggestion of `catchret from %catch to label %foo` instead of the terse `catchret %catch to label %foo`)
================
Comment at: docs/ExceptionHandling.rst:717-719
@@ +716,5 @@
+necessary to recover the nesting that was present in the source. This funclet
+parent relationship is encoded in the IR using tokens produced by the new "pad"
+instructions. The token operand of a "pad" or "ret" instruction indicates which
+funclet it is in, or "none" if it is considered to be in the parent function.
+
----------------
I wonder if we should be more explicit about what "parent" and "in" mean here? To make it clear that we mean
```
try {
} catch { // <-- parent/outer
try {
} catch { // <-- child/inner
}
}
```
and not
```
try {
try {
} catch { // <-- child/inner?
}
} catch { // <-- parent/outer?
}
```
================
Comment at: docs/ExceptionHandling.rst:723-726
@@ +722,6 @@
+their tokens are consumed by other "pad" instructions to establish membership.
+The ``catchswitch`` instruction does not create a funclet, but it produces a
+token that is always consumed by its immediate successor ``catchpad``
+instructions. This ensures that every catch handler modelled by a ``catchpad``
+belongs to exactly one ``catchswitch``, which models the dispatch point after a
+C++ try. The ``terminatepad`` instruction cannot contain lexically nested
----------------
The token on `catchswitch` consumed by `catchpad` seems superfluous -- wouldn't a verifier rule that every `catchpad` have a unique predecessor which is a `catchswitch` enforce the same constraint? `CatchPadInst` could have a `getCatchSwitch()` convenience accessor that grabs the pred's terminator and downcasts it to `CatchSwitchInst`.
================
Comment at: docs/LangRef.rst:5334
@@ +5333,3 @@
+The '``catchswitch``' instruction is used by `LLVM's exception handling system
+<ExceptionHandling.html#overview>`_ describe the set of possible catch handlers
+that may be executed by the :ref:`EH personality routine <personalityfn>`.
----------------
s/describe/to describe/
================
Comment at: docs/LangRef.rst:5345-5346
@@ +5344,4 @@
+The ``default`` argument is the label of another basic block beginning with a
+"pad" instruction, such as ``cleanuppad``, ``terminatepad``, or
+``catchswitch``.
+
----------------
s/such as/one of/? I think that's a complete list.
================
Comment at: docs/LangRef.rst:5348
@@ +5347,3 @@
+
+The ``handlers`` are a list of successor blocks that begin with a
+:ref:`catchpad <i_catchpad>` instruction.
----------------
This reads oddly to me. s/that begin/that each begin/?
================
Comment at: docs/LangRef.rst:5355
@@ +5354,3 @@
+Executing this instruction transfers control to one of the successors in
+``handlers``, if appropriate, or continuing to unwind via the unwind label if
+present.
----------------
s/continuing/continues/
================
Comment at: docs/LangRef.rst:5518-5528
@@ -5643,13 +5517,13 @@
It is undefined behavior to execute a ``cleanupret`` whose ``cleanuppad`` has
not been executed.
It is undefined behavior to execute a ``cleanupret`` if, after the most recent
-execution of its ``cleanuppad``, some ``cleanupret`` or ``cleanupendpad``
+execution of its ``cleanuppad``, any other ``cleanupret``
consuming the same ``cleanuppad`` has already been executed.
It is undefined behavior to execute a ``cleanupret`` if, after the most recent
execution of its ``cleanuppad``, any other ``cleanuppad`` or ``catchpad`` has
been executed but has not had a corresponding
-``cleanupret``/``catchret``/``cleanupendpad``/``catchendpad`` executed.
+``cleanupret``/``catchret`` executed.
----------------
This isn't exactly right without the endpads. I think these three paragraphs can be replaced with something like
The unwind destination ``continue``, if present, must be an EH pad
whose parent is either ``none`` or an ancestor of the ``cleanuppad``
being returned from. This constitutes an exceptional exit from all
ancestors of the completed ``cleanuppad``, up to but not including
the parent of ``continue``.
================
Comment at: docs/LangRef.rst:8627-8632
@@ -8761,8 +8626,8 @@
must have the same exceptional successor.
- It is undefined behavior for control to transfer from a ``cleanuppad`` to a
- ``ret`` without first executing a ``cleanupret`` or ``cleanupendpad`` that
- consumes the ``cleanuppad``.
+ ``ret`` without first executing a ``cleanupret`` that consumes the
+ ``cleanuppad``.
- It is undefined behavior for control to transfer from a ``cleanuppad`` to
- itself without first executing a ``cleanupret`` or ``cleanupendpad`` that
- consumes the ``cleanuppad``.
+ itself without first executing a ``cleanupret`` that consumes the
+ ``cleanuppad``.
----------------
Again, not quite right without the endpads. I think we're looking for something along the lines of
Executing a ``cleanuppad`` instruction constitutes "entering" that pad. The pad
may then be "exited" in one of three ways:
1) explicitly via a ``cleanupret`` that consumes it. Executing such a ``cleanupret``
is undefined behavior if any descendant pads have been entered but not yet
exited.
2) implicitly via a call (which unwinds all the way to the current function's caller),
or via a ``catchswitch``, ``cleanupret``, or ``terminatepad`` that unwinds to caller
3) implicitly via an unwind edge whose destination EH pad isn't a descendant of
the ``cleanuppad``. When the ``cleanuppad`` is exited in this manner, it is
undefined behavior if the destination EH pad has a parent which is not an
ancestor of the ``cleanuppad`` being exited.
Hopefully you can find a less awkward way to say all that. Catchpad should say something similar, and cleanupret and catchret should either refer to this or describe themselves that it's UB to exit the pad that's not the current top of the handler stack. Maybe it would be easiest to explicitly describe that dynamically there's a stack of active handlers, and how they get pushed/popped. I think the langref should somehow explain that each unwind edge exits 0-or-more handlers and then enters exactly 1 handler, that the active handler stack must always correspond to a path from root to some node in the tree encoded by the parent tokens, and that taking an unwind edge which violates those rules is UB.
================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:425
@@ -427,1 +424,3 @@
+ FUNC_CODE_INST_CATCHSWITCH = 53, // CATCHSWITCH: [num,args...] or [num,args...,bb]
+ // 54 is unused.
FUNC_CODE_OPERAND_BUNDLE = 55, // OPERAND_BUNDLE: [tag#, value...]
----------------
Why not use 54 for FUNC_CODE_OPERAND_BUNDLE?
================
Comment at: include/llvm/IR/InstrTypes.h:1140
@@ +1139,3 @@
+
+ /// getNumArgOperands - Return the number of cleanuppad arguments.
+ ///
----------------
s/cleanuppad/funcletpad/
================
Comment at: include/llvm/IR/InstrTypes.h:1151
@@ +1150,3 @@
+
+ /// getArgOperand/setArgOperand - Return/set the i-th cleanuppad argument.
+ ///
----------------
s/cleanuppad/funcletpad/
================
Comment at: include/llvm/IR/Instructions.h:3848
@@ -3827,1 +3847,3 @@
+ Value *getOuterScope() const { return getOperand(0); }
+ void setOuterScope(Value *UnwindCase) { setOperand(0, UnwindCase); }
----------------
s/UnwindCase/OuterScope/?
================
Comment at: include/llvm/IR/Instructions.h:3947
@@ -3975,1 +3946,3 @@
+ void init(Value *OuterScope, BasicBlock *BB, ArrayRef<Value *> Args,
+ const Twine &NameStr);
----------------
Why does this take a NameStr, doesn't it have void type? Where does the NameStr get displayed?
================
Comment at: include/llvm/IR/Instructions.h:4086
@@ +4085,3 @@
+ explicit CleanupPadInst(Value *OuterScope, ArrayRef<Value *> Args,
+ unsigned Values, const Twine &NameStr,
+ Instruction *InsertBefore)
----------------
nit: seems odd to take `Values` as a parameter when it's always `Args.size() + 1`
================
Comment at: include/llvm/IR/Instructions.h:4114
@@ -4126,2 +4113,3 @@
- // Methods for support type inquiry through isa, cast, and dyn_cast:
+ BasicBlock *getCleanupRetUnwindEdge() const;
+
----------------
Naming nit: seems funny to call it `Edge` but return a `Block`. Maybe `getCleanupRetUnwindDest`?
================
Comment at: include/llvm/IR/Instructions.h:4131
@@ +4130,3 @@
+ explicit CatchPadInst(Value *OuterScope, ArrayRef<Value *> Args,
+ unsigned Values, const Twine &NameStr,
+ Instruction *InsertBefore)
----------------
Same as cleanuppad -- seems odd to take a `Values` parameter that can be computed here.
================
Comment at: include/llvm/IR/Instructions.h:4191
@@ -4160,3 +4190,3 @@
public:
- static CatchReturnInst *Create(CatchPadInst *CatchPad, BasicBlock *BB,
+ static CatchReturnInst *Create(Value *CatchPad, BasicBlock *BB,
Instruction *InsertBefore = nullptr) {
----------------
This wasn't explicitly mentioned in the change description -- I assume you just figured making this strongly-typed isn't worth the complexity of supporting operation constraints in the parsers? I don't feel strongly, either way, just want to make sure I understand the change.
================
Comment at: lib/Analysis/CFG.cpp:252-257
@@ +251,8 @@
+ // a nested funclet).
+ // Use a CFG walk driven by a worklist of (block, color) pairs. The "color"
+ // sets attached during this processing to a block which is the entry of some
+ // funclet F is actually the set of F's parents -- i.e. the union of colors
+ // of all predecessors of F's entry. For all other blocks, the color sets
+ // are as defined above. A post-pass fixes up the block color map to reflect
+ // the same sense of "color" for funclet entries as for other blocks.
+
----------------
This paragraph is stale and should be removed.
================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:415-419
@@ -423,7 +414,7 @@
const MachineInstr &MI = *MBBI;
- if (!VisitingInvoke && LastStateChange.NewState != NullState &&
+ if (!VisitingInvoke && LastStateChange.NewState != BaseState &&
MI.isCall() && !EHStreamer::callToNoUnwindFunction(&MI)) {
// Indicate a change of state to the null state. We don't have
// start/end EH labels handy but the caller won't expect them for
// null state regions.
LastStateChange.PreviousEndLabel = CurrentEndLabel;
----------------
It seems wrong to omit start/end labels for non-null states; maybe this BaseState should be left as NullState?
================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:422
@@ -430,3 +421,3 @@
LastStateChange.NewStartLabel = nullptr;
- LastStateChange.NewState = NullState;
+ LastStateChange.NewState = BaseState;
CurrentEndLabel = nullptr;
----------------
same
================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:443-446
@@ -451,7 +442,6 @@
int NewState = StateAndEnd.first;
- // Ignore EH labels explicitly annotated with the null state (which
- // can happen for invokes that unwind to a chain of endpads the last
- // of which unwinds to caller). We'll see the subsequent invoke and
- // report a transition to the null state same as we do for calls.
- if (NewState == NullState)
+ // Ignore EH labels explicitly annotated with the null state.
+ // We'll see the subsequent invoke and report a transition to the null
+ // state same as we do for calls.
+ if (NewState == BaseState)
continue;
----------------
With endpads gone, we should be able to assert `NewState != NullState`, no? Also, comment ("annotated with the null state") doesn't match code ("`== BaseState`") now.
================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:469-473
@@ -478,7 +468,7 @@
// Iteration hit the end of the block range.
- if (LastStateChange.NewState != NullState) {
+ if (LastStateChange.NewState != BaseState) {
// Report the end of the last new state
LastStateChange.PreviousEndLabel = CurrentEndLabel;
LastStateChange.NewStartLabel = nullptr;
- LastStateChange.NewState = NullState;
+ LastStateChange.NewState = BaseState;
// Leave CurrentEndLabel non-null to distinguish this state from end.
----------------
Why would we want to report a change to a non-null base state?
================
Comment at: lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp:228-234
@@ -227,9 +227,9 @@
}
- if (isa<CatchEndPadInst>(I) || isa<CleanupEndPadInst>(I)) {
+ if (isa<CatchSwitchInst>(I)) {
assert(&*BB->begin() == I &&
"WinEHPrepare failed to remove PHIs from imaginary BBs");
continue;
}
- if (isa<CatchPadInst>(I) || isa<CleanupPadInst>(I))
+ if (isa<FuncletPadInst>(I))
assert(&*BB->begin() == I && "WinEHPrepare failed to demote PHIs");
}
----------------
(not part of this change) Why are we still removing PHIs in WinEHPrepare? I thought the RA could do the spilling now.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:86
@@ -114,1 +85,3 @@
+ DenseMap<BasicBlock *, ColorVector> BlockColors;
+ MapVector<BasicBlock *, std::vector<BasicBlock *>> FuncletBlocks;
};
----------------
Why does this need to be a MapVector now? We had nondeterministic iteration within WinEHPrep but deterministic output before, I'm not following what changed.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:180-184
@@ -207,1 +179,7 @@
+ int BaseState = -1;
+ if (FuncletUnwindDest == InvokeUnwindDest) {
+ auto BaseStateI = FuncInfo.FuncletBaseStateMap.find(FuncletPad);
+ if (BaseStateI != FuncInfo.FuncletBaseStateMap.end())
+ BaseState = BaseStateI->second;
+ }
----------------
Clearly I'm just not understanding what the "base states" are. Is that a new thing? Does it apply to CLR, or just C++ and SEH? If I assume it's always empty for CLR, then it looks like this routine just maps each invoke to its unwind dest's state (per the EHPadStateMap), which I think is correct, but the extra logic confused me...
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:416-417
@@ -469,4 +415,4 @@
void llvm::calculateClrEHStateNumbers(const Function *Fn,
WinEHFuncInfo &FuncInfo) {
// Return if it's already been done.
----------------
I think this can be rewritten to be more straightforward now that we have the parent -> child links, since all we need is a topological ordering and so we should be able to just assign pre-order numbers to the handlers. I'm happy to do that as a follow-up change if you'd like; the code you have now, as you pointed out, may or may not be correct (I'd rather just switch to preorder numbers than work through its current correctness in my head).
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1858-1860
@@ -1857,5 +817,2 @@
}
- // FIXME: Check for invokes/cleanuprets/cleanupendpads which unwind to
- // implausible catchendpads (i.e. catchendpad not in immediate parent
- // funclet).
}
----------------
You're deleting this comment, but I think it still applies -- there should be code around here somewhere that checks if an invoke/catchswitch/cleanupret has a bad unwind destination (i.e. an unwind edge which tries to enter multiple funclets at once).
================
Comment at: lib/IR/Instructions.cpp:936
@@ -1011,1 +935,3 @@
+bool CatchSwitchInst::inOutermostScope() const {
+ return isa<ConstantTokenNone>(getOuterScope()) && unwindsToCaller();
}
----------------
I was expecting the outer scopes and their outermostness to be strictly about handler nesting, not about try nesting -- so I wouldn't expect `&& unwindsToCaller()` here. I.e.
```
try {
try {
} catch { // <-- inOutermostScope() == false, but I'd have expected the opposite
}
} catch { // <-- inOutermostScope() == true, which I'd expect
try {
} catch { // <-- inOutermostScope() == false, which I'd expect
}
}
```
Was that intentional?
================
Comment at: lib/IR/Instructions.cpp:994-995
@@ +993,4 @@
+ return CRI->getUnwindDest();
+ // We don't have a cleanupret use, return null. Semantically, this is the
+ // same as unwinding to caller.
+ return nullptr;
----------------
It could be that we unwind to caller via an invoke or whatever, but it could also mean that the cleanup ends in a noreturn call or an infinite loop. (I think returning nullptr is the correct thing here regardless, I'm just quibbling with the comment)
================
Comment at: lib/IR/Instructions.cpp:1002
@@ -1017,1 +1001,3 @@
+ return false;
+ return getCleanupRetUnwindEdge() == nullptr;
}
----------------
ditto
================
Comment at: lib/IR/Verifier.cpp:401-402
@@ -400,4 +400,4 @@
void visitCleanupPadInst(CleanupPadInst &CPI);
- void visitCleanupEndPadInst(CleanupEndPadInst &CEPI);
+ void visitCatchSwitchInst(CatchSwitchInst &CatchSwitch);
void visitCleanupReturnInst(CleanupReturnInst &CRI);
void visitTerminatePadInst(TerminatePadInst &TPI);
----------------
We need all the invokes/catchswitches/terminatepads in (the top-level of) a cleanuppad to unwind to the same place that its cleanuprets do, and we need all the invokes/catchswitches/terminatepads in (the top-level of) a catchpad to unwind to the same place that its catchswitch does. The lang ref should mention this. Checking it in the verifier would require building the block colors, I think maybe it would make more sense to declare it UB and use some sort of implausible unwind pruning in WinEHPrep.
================
Comment at: lib/IR/Verifier.cpp:2907
@@ -2914,2 +2906,3 @@
Assert(F->hasPersonalityFn(),
- "CatchPadInst needs to be in a function with a personality.", &CPI);
+ "CleanupPadInst needs to be in a function with a personality.", &CPI);
+
----------------
s/CleanupPadInst/CatchPadInst/
================
Comment at: lib/IR/Verifier.cpp:2910
@@ +2909,3 @@
+ Assert(isa<CatchSwitchInst>(CPI.getOuterScope()),
+ "CleanupPadInst needs to be directly nested in a CatchSwitchInst.",
+ CPI.getOuterScope());
----------------
s/CleanupPadInst/CatchPadInst/
================
Comment at: lib/IR/Verifier.cpp:2916
@@ -2919,3 +2915,3 @@
Assert(BB->getFirstNonPHI() == &CPI,
- "CatchPadInst not the first non-PHI instruction in the block.",
+ "CleanupPadInst not the first non-PHI instruction in the block.",
&CPI);
----------------
s/CleanupPadInst/CatchPadInst/
================
Comment at: lib/IR/Verifier.cpp:2919
@@ -2922,18 +2918,3 @@
- if (!BB->getSinglePredecessor())
- for (BasicBlock *PredBB : predecessors(BB)) {
- Assert(!isa<CatchPadInst>(PredBB->getTerminator()),
- "CatchPadInst with CatchPadInst predecessor cannot have any other "
- "predecessors.",
- &CPI);
- }
-
- BasicBlock *UnwindDest = CPI.getUnwindDest();
- Instruction *I = UnwindDest->getFirstNonPHI();
- Assert(
- isa<CatchPadInst>(I) || isa<CatchEndPadInst>(I),
- "CatchPadInst must unwind to a CatchPadInst or a CatchEndPadInst.",
- &CPI);
-
- visitTerminatorInst(CPI);
+ visitInstruction(CPI);
}
----------------
There should be a check in here that the associated catchswitch is the unique predecessor (or, if I had my druthers, catchpadinst wouldn't consume a token from the catchswitch, and then we'd want a check here that there is a unique predecessor which is a catchswitch).
================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1110-1115
@@ +1109,8 @@
+ // There is no single parent, inlining will not succeed.
+ if (NumColors > 1)
+ return false;
+ if (NumColors == 1) {
+ CallSiteEHPad = CallSiteColors.front()->getFirstNonPHI();
+ assert(CallSiteEHPad->isEHPad() && "Expected an EHPad!");
+ }
+
----------------
This looks like you're expecting the color set for a call that's not within a funclet to be empty. But wouldn't that case have a single color which is the function entry?
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3282-3283
@@ +3281,4 @@
+ llvm_unreachable("unknown EH pad at unwind dest");
+ if (CPInst->getOuterScope() != DestParentFunclet)
+ return false;
+ }
----------------
I'd expect we're ok if `DestParentFunclet` is any ancestor of `CPInst`, not that we'd need the stricter condition you have here that it is exactly equal to `CPInst`'s parent.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3503-3506
@@ -3502,6 +3516,2 @@
}
- // TODO: If TI is a CatchPadInst, then (BB must be its normal dest and)
- // we can eliminate it, redirecting its preds to its unwind successor,
- // or to the next outer handler if the removed catch is the last for its
- // catchendpad.
}
----------------
I think the opportunity is still there -- we could clip out a `catchswitch` if all its `catchpad`s begin with `unreachable`.
================
Comment at: test/CodeGen/WinEH/wineh-cloning.ll:551-553
@@ -632,20 +550,5 @@
}
-; Make sure we don't clone the catchendpad even though the
-; cleanupendpads targeting it would naively imply that it
-; should get their respective parent colors (catch1 and catch2),
-; as well as its properly getting the root function color. The
-; references from the invokes ensure that if we did make clones
-; for each catch, they'd be reachable, as those invokes would get
-; rewritten
; CHECK-LABEL: define void @test14()
-; CHECK-NOT: catchendpad
-; CHECK: invoke void @h(i32 1)
-; CHECK-NEXT: unwind label %catch.end
-; CHECK-NOT: catchendpad
-; CHECK: invoke void @h(i32 2)
-; CHECK-NEXT: unwind label %catch.end
-; CHECK-NOT: catchendpad
-; CHECK: catch.end:
-; CHECK-NEXT: catchendpad
-; CHECK-NOT: catchendpad
+; CHECK: call void @h(i32 1)
+; CHECK: call void @h(i32 2)
----------------
I'm not sure this is checking anything interesting anymore; maybe just remove this test?
================
Comment at: test/CodeGen/X86/win32-seh-cleanupendpad.ll:1
@@ -1,2 +1,2 @@
; RUN: llc < %s | FileCheck %s
----------------
Seems odd to keep a test called win32-seh-cleanupendpad.ll when you're removing cleanupendpad...
================
Comment at: test/CodeGen/X86/wineh-coreclr.ll:16
@@ -15,3 +15,3 @@
; f(3);
-; } catch (type2) [
+; } catch (type2) {
; f(4);
----------------
:) thanks.
http://reviews.llvm.org/D15139
More information about the llvm-commits
mailing list