[PATCH] D16319: [Inliner/WinEH] Honor implicit nounwinds

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:359
@@ +358,3 @@
+  Value *UnwindDestToken = getUnwindDestTokenHelper(EHPad, MemoMap);
+  assert((UnwindDestToken == nullptr) ^ (MemoMap.count(EHPad) != 0));
+  if (UnwindDestToken)
----------------
I'd use logical not-equal instead of bit-wise xor

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:392-409
@@ +391,20 @@
+  SmallVector<Instruction *, 8> Worklist;
+  for (Instruction *UselessPad = LastUselessPad;;
+       UselessPad = Worklist.pop_back_val()) {
+    assert(!MemoMap.count(UselessPad) || MemoMap[UselessPad] == nullptr);
+    MemoMap[UselessPad] = UnwindDestToken;
+    if (auto *CatchSwitch = dyn_cast<CatchSwitchInst>(UselessPad)) {
+      for (BasicBlock *HandlerBlock : CatchSwitch->handlers())
+        for (User *U : HandlerBlock->getFirstNonPHI()->users())
+          if (isa<CatchSwitchInst>(U) || isa<CleanupPadInst>(U))
+            Worklist.push_back(cast<Instruction>(U));
+    } else {
+      assert(isa<CleanupPadInst>(UselessPad));
+      for (User *U : UselessPad->users())
+        if (isa<CatchSwitchInst>(U) || isa<CleanupPadInst>(U))
+          Worklist.push_back(cast<Instruction>(U));
+    }
+    if (Worklist.empty())
+      break;
+  }
+
----------------
Would it be more idiomatic to start the worklist with `UselessPad` and switch the loop to something like `while (!Worklist.empty())`

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:445-453
@@ +444,11 @@
+        continue;
+      assert(FuncletUnwindMap->count(
+                 isa<CatchPadInst>(FuncletPad)
+                     ? cast<CatchPadInst>(FuncletPad)->getCatchSwitch()
+                     : FuncletPad) &&
+             (*FuncletUnwindMap)[isa<CatchPadInst>(FuncletPad)
+                                     ? cast<CatchPadInst>(FuncletPad)
+                                           ->getCatchSwitch()
+                                     : FuncletPad] == UnwindDestToken &&
+             "must get memoized to avoid confusing later searches");
+    }
----------------
I think this might be easier with `#ifndef NDEBUG`

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:449-452
@@ +448,6 @@
+                     : FuncletPad) &&
+             (*FuncletUnwindMap)[isa<CatchPadInst>(FuncletPad)
+                                     ? cast<CatchPadInst>(FuncletPad)
+                                           ->getCatchSwitch()
+                                     : FuncletPad] == UnwindDestToken &&
+             "must get memoized to avoid confusing later searches");
----------------
If we are in this case, can we also assert that `UnwindDestToken` is not null?

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:616-625
@@ +615,12 @@
+        Value *UnwindDestToken;
+        if (auto *ParentPad =
+                dyn_cast<Instruction>(CatchSwitch->getParentPad())) {
+          // This catchswitch is nested inside another funclet.  If that
+          // funclet has an unwind destination within the inlinee, then
+          // unwinding out of this catchswitch would be UB.  Rewriting this
+          // catchswitch to unwind to the inlined invoke's unwind dest would
+          // give the parent funclet multiple unwind destinations, which is
+          // something that subsequent EH table generation can't handle and
+          // that the veirifer rejects.  So when we see such a call, leave it
+          // as "unwind to caller".
+          UnwindDestToken = getUnwindDestToken(ParentPad, FuncletUnwindMap);
----------------
Does this case only arise when you have an unwind-to-caller catchswitch nested inside an unwind-to-caller catchswitch/cleanuppad?

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:630
@@ +629,3 @@
+        } else {
+          // This catchswitch has no patern to inherit constraints from, and
+          // none of its descendants can have an unwind edge that exits it and
----------------
patern -> parent?


http://reviews.llvm.org/D16319