[PATCH] D24117: Fix inliner funclet unwind memoization

[Joseph Tremoulet via llvm-commits]

JosephTremoulet marked 3 inline comments as done.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:413
@@ +412,3 @@
+  // exited, it must be the case that, walking downward from LastUselessPad,
+  // visiting just those nodes which have not been mapped to an unwind dest
+  // by getUnwindDestTokenHelper (the nullptr TempMemos notwithstanding, since
----------------
Meant to say "dest"; fixed.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:420
@@ -392,2 +419,3 @@
   while (!Worklist.empty()) {
     Instruction *UselessPad = Worklist.pop_back_val();
+    auto Memo = MemoMap.find(UselessPad);
----------------
Assuming that "give it `LastUselessPad`" means "map it to `UnwindDestToken`", that sounds correct, but I don't see why we'd want to have `getUnwindDestTokenHelper` duplicate its work, especially since doing so at every link in the chain would be quadratic.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:423-431
@@ +422,11 @@
+    if (Memo != MemoMap.end() && Memo->second) {
+      // Here the name 'UselessPad' is a bit of a misnomer, because we've found
+      // that it is a funclet that does have information about unwinding to
+      // a particular destination; its parent was a useless pad.
+      // Since its parent has no information, the unwind edge must not escape
+      // the parent, and must target a sibling of this pad.  This local unwind
+      // gives us no information about EHPad.  Leave it and the subtree rooted
+      // at it alone.
+      assert(getParentPad(Memo->second) == getParentPad(UselessPad));
+      continue;
+    }
----------------
Assertion updated, testcase added.

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




https://reviews.llvm.org/D24117