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

[Andy Kaylor via llvm-commits]

andrew.w.kaylor 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) {
----------------
JosephTremoulet wrote:
> `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.
I suppose you're right.  I think I can handle that just by replacing the assert with a comment.  The unused PHI's should go away when we remove the block.  Alternatively, I could add something to go through any PHI's and assert that they have no uses.

================
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));
----------------
JosephTremoulet wrote:
> 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. 
Good catch.  I was just thinking it couldn't be a non-PHI value in the empty cleanup pad block.  I think you're right that this should be easy to handle.

================
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),
----------------
JosephTremoulet wrote:
> 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
> 
Alright, that seems like sound reasoning to me.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3074
@@ +3073,3 @@
+          }
+          SrcPN->eraseFromParent();
+          break;
----------------
JosephTremoulet wrote:
> 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.
OK.

================
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);
----------------
JosephTremoulet wrote:
> Likewise, I think the getBasicBlockIndex check here is superfluous.
I'm assuming "likewise" here is referring to your comments at line 3069.  So you're saying that any predecessor of the unwindpad we're removing could not also have been a predecessor of UnwindDest., right?

So, any PHINode that was present in the cleanuppad block we are removing must necessarily have undefined values for any other predecessor of UnwindDest.  That sounds correct.  Am I correct in thinking that because the PHI value from BB could not have been used in the pre-optimized control flow there must be something (a PHI-dependent branch, perhaps) still in place that will prevent the undef value being inserted here from being referenced?


Repository:
  rL LLVM

http://reviews.llvm.org/D12434