[PATCH] D11097: New EH representation for MSVC compatibility

[Joseph Tremoulet]

JosephTremoulet added inline comments.

================
Comment at: lib/IR/BasicBlock.cpp:337
@@ +336,3 @@
+bool BasicBlock::isSplittable() const {
+  return !isa<CatchEndPadInst>(getFirstNonPHI());
+}
----------------
majnemer wrote:
> JosephTremoulet wrote:
> > Surprised this is just CatchEndPad; don't CatchPad and TerminatePad have the same restrictions?
> `CatchPad` can be split (for the purposes of `SplitBlockPredecessors`) by:
> 
>   # Creating a new BB
>   # Cloning the `CatchPad` into the new BB
>   # Moving relevant `PHI` nodes to the new BB
>   # Hooking up relevant predecessors to unwind to the new BB
> 
> `TerminatePad` can be "split" using a similar mechanism.
> 
> The name of the function might need some work...
> The name of the function might need some work...
Yes, I think the name was throwing me off.  Looking at what `SplitBlockPredecessors` does, this makes sense.  Which suggests the name `canSplitPredecessors`

> CatchPad can be split (for the purposes of SplitBlockPredecessors) by:
> 1.Creating a new BB
> 2.Cloning the CatchPad into the new BB
> 3.Moving relevant PHI nodes to the new BB
> 4.Hooking up relevant predecessors to unwind to the new BB
Some questions about this:
1. Will callers find this useful, or are they expecting that the new BB will be one they can insert instructions into prior to branching to the original BB?
2. Do you need to add a step for inserting another BB, successor to the new catchpad, with a catchret?
3. Could the same approach work equally well for catchendpad except changing #2 from "cloning the CatchPad into the new BB" to "inserting a new CatchPad into the new BB"?  Is the problem that we don't have a general way to compute the arguments of a catch-all CatchPad?



http://reviews.llvm.org/D11097