[PATCH] D11097: New EH representation for MSVC compatibility

[David Majnemer]

majnemer added a comment.

In http://reviews.llvm.org/D11097#209327, @reames wrote:

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


http://reviews.llvm.org/D11041 was an older version of this patch, I'd ignore it.

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





================
Comment at: docs/LangRef.rst:8421
@@ -8054,3 +8420,3 @@
 Intrinsic Functions
 ===================
 
----------------
reames wrote:
> 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.
I don't think that the LangRef is the right place to discuss which set of EH-related IR is most appropriate for a frontend.  I think it would make more sense in ExceptionHandling.rst or maybe a new file in the docs/Frontend directory.

We introduced these new instructions because of constraints imposed by our personality routine. For example:
-  we need to be able to statically determine which invokes occurred in a catch block.
- it is impossible to lower `@llvm.eh.typeid.for` for our personality routine.

These constraints make landingpad/resume inappropriate, especially in the face of optimizations.  These new instructions allow us maintain enough information in IR-form that we can lower it into a representation our personality will be happy with.

For example, we can easily find all the code for a try body's catch handlers by examining the unwind labels and operands of its relevant `CatchBlockInst`s and tracing execution to relevant `CatchRetInst`s.  Such an operation would be impossible in the landingpad scheme because control flow can leave "exceptional" code back to "normal" code through a branch instruction.

================
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()) {
----------------
reames wrote:
> 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?
I'm all for changing the `Block` suffix given a reasonable suggestion.

It is on instruction because we have instructions called `CatchBlockInst`, `CleanupBlockInst`, etc.  The predicate answers the question if it is one of these "FooBlock" instructions.

================
Comment at: lib/IR/Verifier.cpp:2819
@@ -2797,1 +2818,3 @@
 
+  if (!LandingPadResultTy)
+    LandingPadResultTy = LPI.getType();
----------------
reames wrote:
> 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.  
Sure, removed.

================
Comment at: lib/IR/Verifier.cpp:2900
@@ +2899,3 @@
+    if (isa<CatchBlockInst>(PredBB->getTerminator()))
+      ++CatchBlocksSeen;
+
----------------
reames wrote:
> Why not exit on 2nd one found?
Just to make the code simpler, it would be optimizing for a case that would never happen in practice.

================
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.",
----------------
reames wrote:
> 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.
The instructions have "Block" in the name, hence "Block" in the function's name.

================
Comment at: lib/IR/Verifier.cpp:2953
@@ +2952,3 @@
+
+void Verifier::visitTerminateBlockInst(TerminateBlockInst &TBI) {
+  BasicBlock *BB = TBI.getParent();
----------------
reames wrote:
> TerminateBlock is a highly confusing name if this is EH specific.  "Doesn't every block have a terminator?"
I'm open to suggestions, it's the best name we could come up with.


http://reviews.llvm.org/D11097