[PATCH] D12434: [WinEH] Teach SimplfyCFG to eliminate empty cleanup pads.

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added inline comments.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3045
@@ -2942,2 +3044,3 @@
 
-  // The landingpad is now unreachable.  Zap it.
+  assert(UnwindDest || BB->getFirstNonPHI() == BB->begin());
+  if (UnwindDest) {
----------------
`assert` seems too strong here.  Any PHI in an empty unwinds-to-caller cleanuppad must be dead (have no uses), but it would still be legal IR, right?  I'd think that in the unwinds-to-caller case this should simply erase any PHIs it finds.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3058-3059
@@ +3057,4 @@
+        if (DestPN->getIncomingBlock(Idx) == BB) {
+          // The value has to be a PHI because we've check that there are no
+          // other instructions in the block we are removing.
+          PHINode *SrcPN = cast<PHINode>(DestPN->getIncomingValue(Idx));
----------------
It could be a non-PHI value that dominates the empty cleanup, though, couldn't it?  So something like

```
struct S { ... }; // has empty destructor
void foo() {
  int state = bar();
  try {
    {
      S s;
      may_throw();
    } // empty cleanup for s here
    state = 2;
    may_throw();
  } catch (...) {
    // This is UnwindDest and should have a PHI for state where IncomingBlock is the empty cleanuppad
    // and IncomingValue is the call to bar, not a PHI in the empty cleanup.
    code;
  }
}
```

or with a constant:

```
struct S { ... }; // has empty destructor
void foo() {
  int state = 1;
  try {
    {
      S s;
      may_throw();
    } // empty cleanup for s here
    state = 2;
    may_throw();
  } catch (...) {
    // This is UnwindDest and should have a PHI for state where IncomingBlock is the empty cleanuppad
    // and IncomingValue is 1, not a PHI in the empty cleanup
    code;
  }
}
```

Happily, I think in these cases you can just take the current incoming value in `DestPN` and associate it with `UnwindDest`'s new predecessors. 

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3066-3069
@@ +3065,6 @@
+               SrcIdx != SrcE; ++SrcIdx) {
+            // If the destination PHI node already has an entry for this block
+            // we can just leave that.  Otherwise, add an incoming value.
+            if (DestPN->getBasicBlockIndex(SrcPN->getIncomingBlock(SrcIdx))
+                  == -1) {
+              DestPN->addIncoming(SrcPN->getIncomingValue(SrcIdx),
----------------
I don't think the destination PHI node can already have an entry for this block:
  - `SrcPN` was a PHI in a cleanuppad, so `SrcPN->getIncomingBlock(SrcIdx)` must have reached the cleanuppad by an unwind edge
  - `DestPN` is in `UnwindDest`, which must be an EH pad and therefore all its predecessors must reach `UnwindDest` by an unwind edge
  - No instruction has multiple unwind edges, so nothing could have unwind edges to both `UnwindDest` and the empty cleanup


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3074
@@ +3073,3 @@
+          }
+          SrcPN->eraseFromParent();
+          break;
----------------
I think it's possible that SrcPN could have another use on another PHI in UnwindDest, and/or that it could have a non-PHI use.  So I don't think you want to erase it here, I think instead you want the loop below to check if it has any uses and erase it if not.

Actually, you may want to move the loop below outside the `if(UnwindDest)` and let it be the thing that erases useless PHIs when the empty cleanup unwinds to caller.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3089
@@ +3088,3 @@
+      for (auto *pred : predecessors(UnwindDest))
+        if (PN->getBasicBlockIndex(pred) == -1 && pred != BB)
+          PN->addIncoming(UndefValue::get(PN->getType()), pred);
----------------
Likewise, I think the getBasicBlockIndex check here is superfluous.


Repository:
  rL LLVM

http://reviews.llvm.org/D12434