[PATCH] D11097: New EH representation for MSVC compatibility

[Philip Reames]

reames added a comment.

In http://reviews.llvm.org/D11097#209367, @majnemer wrote:

> @reames thinking about it a little more, would you be happier with `CatchPadInst` instead of `CatchBlockInst`, etc. and `isEHPad` instead of `isEHBlock`?


That would help.  Reusing block causes confusion with the widespread use of "block" to refer to a basic block in the code base.

More generally, consistency in the terminology would be a good thing.  Let me lay out my current understanding:

- A X-"pad" is a place the exceptional control flow might resume.
- A catch is a specific type of "pad" which models a language/ABI level catch clause.  A catch is exception type specific.
- A cleanup is a specific type of "pad" which models scope exit without an explicit exception type.
- Both catch and cleanup are statically scoped.  There's a begin and end construct for each.

In this context, what is a terminateblock?  It's clearly a "pad", but why is not just a cleanup?

On the topic of motivation, I still feel like I'm missing a lot.  In particular, it feels like the catchpad, cleanuppad, and terminatepad are really close to what's already described by landingpad(*).  I get that you need to prevent merging of catch blocks, but why isn't a single "pad fence" at the end of each clause which is unmergeable solve that problem?

- We might end up changing how you describe a catch clause, but the idea is already there.  You do seem to need different arguments then the existing catch clause bits.

p.s. For the record, I'm asking for my own understanding not because I oppose the proposal.


================
Comment at: docs/LangRef.rst:8421
@@ -8054,3 +8420,3 @@
 Intrinsic Functions
 ===================
 
----------------
majnemer wrote:
> 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.
I agree that the LangRef would be the wrong place for this.  I don't really care where it ends up, but I do want to see the motivation documented.

In particular, when should the older scheme be used vs this one?  Are you planning on deprecating the old mechanism?  Is this essentially Windows only?  I honestly couldn't answer any of those questions if asked today.

================
Comment at: lib/IR/Verifier.cpp:2900
@@ +2899,3 @@
+    if (isa<CatchBlockInst>(PredBB->getTerminator()))
+      ++CatchBlocksSeen;
+
----------------
majnemer wrote:
> 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.
I was only asking from the perspective of code clarity.  Having the extra counter variable makes me thing it's going to be used for something.  


http://reviews.llvm.org/D11097