[PATCH] [WinEH] Demote values live across exception handlers up front

[Reid Kleckner]

The next patch has a lot of changes because I realized I had to do more work for phi nodes using constants in EH return points.


================
Comment at: lib/CodeGen/WinEHPrepare.cpp:383
@@ +382,3 @@
+    auto *Br = dyn_cast<BranchInst>(BB->getTerminator());
+    if (Br && Br->getPrevNode() &&
+        match(Br->getPrevNode(), m_Intrinsic<Intrinsic::eh_endcatch>()))
----------------
andrew.w.kaylor wrote:
> Can you add a comment here explaining that the blocks have been split to ensure that the endcatch call will immediately precede the terminator?
Sure.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:427
@@ +426,3 @@
+        // The next BB is normal control flow.
+        Worklist.insert(BB->getTerminator()->getSuccessor(0));
+        break;
----------------
andrew.w.kaylor wrote:
> This doesn't seem right.  A nested landing pad block generally begins with a call to endcatch, but it isn't a normal block.
I think we should stop emitting those endcatch calls in the frontend. I don't think they help identify regions to outline. They way I see it, barring code commoning optimizations, the handlers are just the BBs delimited by the begincatch / endcatch calls. A nested landingpad simply starts a new set of handlers.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:460
@@ +459,3 @@
+        if (auto *Arg = dyn_cast<Argument>(Op)) {
+          if (IsEHBB) {
+            DEBUG(dbgs() << "Demoting argument " << *Arg
----------------
andrew.w.kaylor wrote:
> Does this demote arguments in EH blocks even if the value is defined in an EH block?
> 
> Also, isn't this going to demote the landing pad values which we will later promote?
This all runs before outlining, so the only arguments it will demote are ones in the parent function.

This shouldn't demote selector and ehptr values because they typically aren't live *out* of a landingpad. Even if we demote them by accident in a corner case, I'm not worried about the overhead, so long as we don't demote and then promote in the common cases.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:508
@@ +507,3 @@
+    }
+    new StoreInst(Arg, Slot, AllocaInsertPt);
+  }
----------------
andrew.w.kaylor wrote:
> Is this inserting the store in the entry block?  That doesn't sound right.
Why not? Arguments are SSA values that are defined once at function entry. This is only less than optimal when you have paths through the function that don't reach an invoke with a handler using the argument.

We could try to sink the store somewhere closer to a dominating invoke to stay out of any fast paths through the funciton, but we'd want to stay hoisted out of any loop bodies. This analysis seems hard and not really worth it. If you're still concerned, an easier direction might be to have llvm.argument.escape and llvm.argument.recover intrinsics to recover the address of the argument and ensure that the parent homes escaped register parameters.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:452-457
@@ +451,8 @@
+      for (Value *Op : I.operands()) {
+        // Don't demote static allocas, constants, and labels.
+        if (isa<Constant>(Op) || isa<BasicBlock>(Op))
+          continue;
+        auto *AI = dyn_cast<AllocaInst>(Op);
+        if (AI && AI->isStaticAlloca())
+          continue;
+
----------------
majnemer wrote:
> rnk wrote:
> > majnemer wrote:
> > > Is it possible to demote a dynamic alloca in the exceptional path?
> > Yes, it will produce a dangling pointer. It has the same semantics as the source one would write for it:
> > ```
> > void f() {
> >   int *data = nullptr;
> >   try {
> >     throw 42;
> >   } catch (int e) {
> >     data = (int*)alloca(e);
> >   }
> >   data[0] = 42; // store to dead stack memory
> > }
> > ```
> > Rejecting this isn't our job, it's Clang's.
> What if the programmer wasn't at fault?
> 
> Let's say we had:
>   int n;
>   void f() {
>     int *x = (int *)alloca(n);
>     try {
>       throw 42;
>     } catch (int e) {
>       do_something(x);
>       return;
>     }
>   }
> 
> Is it unreasonable for LLVM to sink the dynamic alloca into the catch block?
I think in general that's too hard for LLVM. If you add a cleanup to your example, do_something(x) could escape x, and then the cleanup would run in the parent frame and use the escaped x, which is now dead.

What's wrong with leaving it alone and calling DemoteRegToStack on the dynamic alloca? If it doesn't do the right thing (I haven't tested it) it sounds like a bug in DemoteRegToStack.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:521-522
@@ +520,4 @@
+    for (Instruction &Inst : *LPadBB) {
+      if (auto *IntrinCall = dyn_cast<IntrinsicInst>(&Inst)) {
+        if (IntrinCall->getIntrinsicID() == Intrinsic::eh_actions)
+          return false;
----------------
majnemer wrote:
> rnk wrote:
> > majnemer wrote:
> > > You could make this just:
> > >   if (match(&Inst, m_Intrinsic<Intrinsic::eh_actions>())
> > >     return false;
> > Won't that require that the intrinsic call has no arguments?
> No: http://llvm.org/docs/doxygen/html/PatternMatch_8h_source.html#l01193
done

http://reviews.llvm.org/D9158

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/