[PATCH] D15517: [WinEH] Use operand bundles to describe call sites

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added inline comments.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:804-809
@@ +803,8 @@
+
+          // Get a pointer to the new call.
+          TI = BB->getTerminator();
+          BasicBlock::iterator CallI = std::prev(TI->getIterator());
+          auto *CI = cast<CallInst>(&*CallI);
+
+          changeToUnreachable(CI, /*UseLLVMTrap=*/false);
+        } else if (Personality == EHPersonality::MSVC_CXX && CleanupPad) {
----------------
I think you should do this part for `CallInst` uses too.

================
Comment at: lib/IR/Verifier.cpp:2397-2398
@@ +2396,4 @@
+      FoundFuncletBundle = true;
+      Assert(BU.Inputs.size() == 1,
+             "Expected exactly one funclet operand bundle input", I);
+    }
----------------
Want to check that it has `token` type?  Or specifically that it is a `FuncletPadInst`?

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1419
@@ -1425,3 +1418,3 @@
 
   // Update the lexical scopes of the new funclets.  Anything that had 'none' as
   // its parent is now nested inside the callsite's EHPad.
----------------
s/funclets/funclets and callsites/?

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1434-1438
@@ +1433,7 @@
+
+        // Skip call sites which are nounwind intrinsics.
+        auto *CalledFn =
+            dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
+        if (CalledFn && CalledFn->isIntrinsic() && CS.doesNotThrow())
+          continue;
+
----------------
If you agree with my feedback in WinEHPrepare about making mis-annotated calls equally UB to mis-annotated invokes (and it seems to me they should agree), then this part will need to agree.  It might make sense to have nounwind intrinsics be a special case (and the only one) in both places.  Or neither.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1454
@@ +1453,3 @@
+        }
+        NewInst->takeName(I);
+        I->replaceAllUsesWith(NewInst);
----------------
The `Create` overloads you call a few lines above pass the old instruction's name as the name argument for the new instruction.  Does `takeName` do something different than (but still compatible with) that?

Also, supposing one of these calls/invokes has debug info (line number etc) attached, is what's here sufficient to propagate it to the new one?


http://reviews.llvm.org/D15517