[PATCH] D24117: Fix inliner funclet unwind memoization

Joseph Tremoulet via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 2 10:04:52 PDT 2016

JosephTremoulet marked an inline comment as done.

Comment at: lib/Transforms/Utils/InlineFunction.cpp:442-444
@@ -395,3 +441,5 @@
+    // the MemoMap on that invocation, which isn't the case if we got here.
+    assert(!MemoMap.count(UselessPad) || TempMemos.count(UselessPad));
     MemoMap[UselessPad] = UnwindDestToken;
     if (auto *CatchSwitch = dyn_cast<CatchSwitchInst>(UselessPad)) {
       for (BasicBlock *HandlerBlock : CatchSwitch->handlers())
majnemer wrote:
> JosephTremoulet wrote:
> > Short answer: The set of nodes that it was necessary for `getUnwindDestTokenHelper` to visit in order to prove `LastUselessPad` useless is a superset of the nodes that are sufficient for it to prove `UselessPad` useless.
> > 
> > Long answer:
> > Let's say a funclet/pad/node is "useless" if neither it nor its dependents have an unwind edge that escapes it (disregarding the can't-be-trusted annotations of calls-that-aren't-invokes and unwinds-to-caller catchswitches) -- i.e. a node that `getUnwindDestTokenHelper` should return `nullptr` for.
> > 
> > For any useless node x, define the "witnesses" of x to be the smallest set satisfying two rules:
> >  - x is a witness of x
> >  - for any useless node y which is a witness of x, all of y's children are witnesses of x
> > 
> > Claim: `getUnwindDestToken` visited all witnesses of `LastUselessPad`, and put entries in `MemoMap` for all its non-useless witnesses.
> > Proof:
> > 
> > 1. Note that `getUnwindDestTokenHelper` only returns `nullptr` when it exhausts its worklist, and that it was called and returned `nullptr` for `LastUselessPad`, so we can assume it exhausted its worklist when called for `LastUelessPad`.
> > 
> > 2. Note that whenever `getUnwindDestTokenHelper` pulls a useless node off its worklist and processes it, there's no `cleanupret`/`invoke`/non-UnwindToCaller-`catchswitch` to find, so it will put all of that node's children on the worklist (unless there's an entry in the `MemoMap` mapping that node to `nullptr`, which only happens if a previous invocation returned `nullptr` for that node, in which case the previous invocation put the children on the worklsit and exhausted the worklist, so the same argument applies).
> > 
> > 3. Since `getUnwindDestTokenHelper` exhausted its worklist, and the children of each processed useless node got queued, the children of each processed useless node got processed as well.
> > 
> > 4. The witnesses of `LastUselessPad` are `LastUselessPad` itself (which was the first node processed by `getUnwindDestTokenHelper`) and the children of useless witnesses of `LastUselessPad` (which are processed by induction using #3 above), so all the witnesses are visited.
> > 
> > 5. The witnesses of `LastUselessPad` which are not themselves useless must have non-null `MemoMap` entries.  Consider the path from such a pad (let's call it `UsefulWitness`) to the topmost self-or-descendant with an unwind edge exiting `UsefulWitness`.
> > 
> >  5a. If `UsefulWitness` itself has an exiting unwind edge, it would be discovered when `UsefulWitness` is processed (and we know it's processed by #4).
> > 
> >  5b. If `UsefulWitness` itself has no such exiting edge, its children will be queued, and since we know the worklist will be exhausted, those children will also be processed.
> > 
> >  5c. Since `UsefulWitness` is not useless, it must have some descendant with an edge that exits it, so by repeated iteration of the logic in 5a/5b, we'll process such a node.  When we do process it, we'll create `MemoMap` entries for all exited pads, which must include `UsefulWitness`
> > 
> > qed
> > 
> > Now, this walk pushing `UnwindDestToken` down through useless nodes must visit exactly the witnesses of `LastUselessPad`  (the structure of the walk follows the definition of witness, and from the Claim above we know that the witnesses aren't missing `MemoMap` entries), so `UselessPad` must really be useless.
> > 
> > Right?
> > 
> > 
> Would that imply that if `UnwindDestToken` is `nullptr` that we can assert that the `catchswitch`/`cleanuppad` both unwind to caller?  Would it also imply that if `UnwindDestToken` is not null that it matches the unwind destination of the `catchswitch`/`cleanuppad`?
> If so, could we have asserts to that effect?
If you're talking specifically about getting here with a `UselessPad` that has no entry in the `MemoMap` or is mapped to `nullptr`, then we can assume that if it is a `catchswitch` it's marked unwind-to-caller, that if it is a `cleanuppad` it has no cleanupret, and that in either case it has no `invoke` users, all of this regardless of the value of `UnwindDestToken`.  Should I add assertions to that effect?


More information about the llvm-commits mailing list