[PATCH] D11097: New EH representation for MSVC compatibility

[Philip Reames]

reames added a comment.

What is the relation between this patch and http://reviews.llvm.org/D11041?  They seem to include some of the same code.

A couple of specific comments, but nothing approaching a full review.


================
Comment at: docs/LangRef.rst:8421
@@ -8054,3 +8420,3 @@
 Intrinsic Functions
 ===================
 
----------------
Is there anywhere in the docs that give context for why these instructions are needed and when they should be used instead of landingpad?  If not, this should be done before checkin.  Despite the discussion on llvmdev, I still don't feel like I understand the use case enough to really review the design.

================
Comment at: include/llvm/IR/Instruction.h:392
@@ +391,3 @@
+  /// \brief Return true if the instruction is a variety of EH-block.
+  bool isEHBlock() const {
+    switch (getOpcode()) {
----------------
As mentioned elsewhere, I find the name "Block" confusing in this function name.  Please change.

Actually, why is this a predicate on the instruction at all?  Isn't this a property of the containing block?

================
Comment at: lib/IR/Verifier.cpp:2819
@@ -2797,1 +2818,3 @@
 
+  if (!LandingPadResultTy)
+    LandingPadResultTy = LPI.getType();
----------------
This looks like a fix for an existing deficiency in the verifier right?  Not specific to the new instructions?  If so, please submit this separately.  

================
Comment at: lib/IR/Verifier.cpp:2867
@@ +2866,3 @@
+
+  // The catchblock instruction must be the first non-PHI instruction in the
+  // block.
----------------
The property of being a single instruction is a block is repeated in several of these.  It would make sense to have this checked in a common helper routine which includes the terminator checks as well.  

================
Comment at: lib/IR/Verifier.cpp:2900
@@ +2899,3 @@
+    if (isa<CatchBlockInst>(PredBB->getTerminator()))
+      ++CatchBlocksSeen;
+
----------------
Why not exit on 2nd one found?

================
Comment at: lib/IR/Verifier.cpp:2909
@@ +2908,3 @@
+    Assert(
+        I->isEHBlock() && !isa<LandingPadInst>(I),
+        "CatchEndBlock must unwind to an EH block which is not a landingpad.",
----------------
The use of the name "isEHBlock" on an *Instruction* seems highly misleading.  Please change this.  That function name on a BasicBlock makes sense, but not on a Instruction.

================
Comment at: lib/IR/Verifier.cpp:2953
@@ +2952,3 @@
+
+void Verifier::visitTerminateBlockInst(TerminateBlockInst &TBI) {
+  BasicBlock *BB = TBI.getParent();
----------------
TerminateBlock is a highly confusing name if this is EH specific.  "Doesn't every block have a terminator?"


http://reviews.llvm.org/D11097