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

Andy Kaylor via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 2 14:58:53 PDT 2015


andrew.w.kaylor added inline comments.

================
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:
> 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
>   }
> ```
It turns out that by this point I've already updated the terminators for BB's predecessors so they are included in this loop.  This check is necessary to distinguish between blocks that previous unwound to BB (and thus already have an entry in the PHI node that we are sinking) and blocks that unwound to UnwindDest even before the transformation (and thus need an undef entry in the PHI node we are sinking).


Repository:
  rL LLVM

http://reviews.llvm.org/D12434





More information about the llvm-commits mailing list