[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) {
----------------
andrew.w.kaylor wrote:
> 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.
>  The unused PHI's should go away when we remove the block.

Oh, I didn't know it worked like that.  Feel free to ignore my suggestion to explicitly remove them then.

================
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);
----------------
andrew.w.kaylor wrote:
> 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?
> So you're saying that any predecessor of the unwindpad we're removing could not also have been a predecessor of UnwindDest., right?

right.

> 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?

Trying to come up with a justification for that claim, I instead convinced myself that you should actually be using the PHI itself as the incoming value, rather than undef:
 You'll only insert a new incoming value if UnwindDest had some predecessor other than the empty cleanup.
 1) If the empty cleanup didn't dominate that other predecessor, then:
    - the empty cleanup (where the PHI used to be) can't dominate anything reachable from UnwindDest
    - then since UnwindDest was the empty cleanup's only successor, that means the empty cleanup didn't dominate anything but itself
    - and since the empty cleanup was empty, there weren't any uses there.
    - so in this case, the only place where the empty cleanup PHI could have had a use (because defs have to dominate uses) is a PHI in UnwindDef.
    - but if there was already a PHI in UnwindDef then it already had values for the other predecessors, so this isn't a case where you'll be inserting a new incoming value here.
    - so case 1 is contradictory and impossible.
 2) If the empty cleanup did dominate that other predecessor, then
     1. if the other predecessor is unreachable, then it doesn't really matter what you put there
     2. if the other predecessor is reachable, then its unwind edge is a back-edge, and since the original PHI wouldn't have changed value around that back-edge we need the new PHI to preserve its value around that back-edge.

So the only possible case is 2b, and the PHI needs a self-reference on the backedge.  I don't think we can get that from C++ because it would have to look something like this and the 'goto' there is illegal:
```
struct S { ~S() { } };

void may_throw();
void use(int);

void foo() {
  int x = 1;
  try {
    {
      S s;
      may_throw();
      x = 2;
      may_throw();
    } // empty cleanup for S with phi for x here
    return;
  loop_bottom:
    may_throw();
    return;
  } catch (...) {
    // predecessors are the empty cleanup and loop_bottom
    // no phi here before optimization
    // phi for x will be moved here by optimization
    use(x); // use of the phi that gets moved
  }
  goto loop_bottom;
}
```

but at the LLVM IR level I don't see anything that would prohibit someone from creating
```
  define void foo() personality whatever {
entry:
    invoke void may_throw()
      to label %invoke.cont unwind label %cleanup
invoke.cont:
    invoke void may_throw()
      to label %exit unwind label %cleanup
cleanup:
  %x = PHI i32 [ 1, %entry ], [ 2, %invoke.cont ]
  %clean = cleanuppad []
  cleanupret %clean unwind label %catch
catch:
  %cat = catchpad []
      to label %do_catch unwind label %endcatch
do_catch:
  call void use(i32 %x)
  catchret %cat to label %loop_bottom
loop_bottom:
  invoke void f()
    to label %exit unwind label %catch
endcatch:
  catchendpad unwind to caller
exit:
  ret void
  }
```


Repository:
  rL LLVM

http://reviews.llvm.org/D12434