[PATCH] D24117: Fix inliner funclet unwind memoization

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
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())
----------------
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?


https://reviews.llvm.org/D24117