[PATCH] D24117: Fix inliner funclet unwind memoization

[David Majnemer via llvm-commits]

majnemer added a comment.

Thanks for working on this!


================
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 desti
+  // by getUnwindDestTokenHelper (the nullptr TempMemos notwithstanding, since
----------------
desti -> destination?

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:423-431
@@ +422,11 @@
+    if (Memo != MemoMap.end() && Memo->second) {
+      // EHPad must have a descendant that unwinds to a different descendant
+      // of EHPad.  This local unwind gives us no information about EHPad.
+      // Leave it and the subtree rooted at it alone.
+#ifndef NDEBUG
+      auto *UnwindAncestor = getParentPad(Memo->second);
+      while (UnwindAncestor && UnwindAncestor != EHPad)
+        UnwindAncestor = getParentPad(UnwindAncestor);
+      assert(UnwindAncestor == EHPad);
+#endif
+      continue;
----------------
JosephTremoulet wrote:
> Oops, I think that the `continue` here is right but the debug check is over-zealous -- this might be a local unwind off in some cousin of `EHPad` in the case that `EHPad` has useless ancestors.  I think instead that since `UselessPad`'s parent has no information, we can assert that the unwind dest is a sibling of `UselessPad` (since otherwise the exiting would have needed to propagate up to the parent).  Will add testcase to verify and update.
I think I reached the same conclusion wrt a `continue` here.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:442-444
@@ -395,2 +441,5 @@
+    // LastUselessPad, which would imply that EHPad was mapped to nullptr in
+    // 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)) {
----------------
What if `UselessPad` is a descendent of `EHPad` which we never visited in `getUnwindDestTokenHelper`?


https://reviews.llvm.org/D24117