[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:
> 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?
I think it would make it clear that it is safe to propagate `UnwindDestToken` downwards.


https://reviews.llvm.org/D24117