Jump threading: avoid trying to split edge into landingpad block (PR27840)

[hans]

Summary

Splitting the edge is nontrivial because of the landing pad, and we would
currently assert trying to do it.

[nlewycky]

I'm concerned that the fix is too narrow. There's 10 callers to llvm::SplitEdge in LLVM and haven't audited them so I don't know whether they also check whether they also do this check. It's certainly not in the comment on SplitEdge that it's allowed to assert if you pass it a landing pad block.

[hans]

In D24680#545412, @nlewycky wrote:

I'm concerned that the fix is too narrow. There's 10 callers to llvm::SplitEdge in LLVM and haven't audited them so I don't know whether they also check whether they also do this check. It's certainly not in the comment on SplitEdge that it's allowed to assert if you pass it a landing pad block.

My thinking was that it seems what jump threading is trying to do with a "branch on xor" pattern isn't going to work with a landingpad anyway, and this would fix the crash that people run into in practice.

I agree it doesn't fix the wider problem of what to do when SplitEdge fails, but it seems like a good improvement to me.

[hans]

Ping?

[hans]

Ping?

Also +Kyle, who I think has looked at jump threading recently.

While I agree there's a bigger problem with SplitEdge failing, I'd like to commit this fix as the transformation this code is trying to do clearly won't work for landingpads, and users are getting crashes on this in the wild.

[iteratee]

I didn't look very closely at jump threading, but nick said to land this patch via email on Sep 23, so I'm comfortable accepting it based on that.

[hans]

Sorry for the mess here.

I missed Nick's emailed replies because they didn't show up on Phabricator, and while they did go to the mailing list, I wasn't on the to: or cc: line so my filter didn't catch it.

I'll land this and keep the PR open as suggested by Nick.

[hans]

r283129

lib/Transforms/Scalar/JumpThreading.cpp

if (isa<ConstantInt>(BO->getOperand(0)) ||
       isa<ConstantInt>(BO->getOperand(1)))
     return false;
 
   // If the first instruction in BB isn't a phi, we won't be able to infer
   // anything special about any particular predecessor.
   if (!isa<PHINode>(BB->front()))
     return false;
 
+  // If this BB is a landing pad, we won't be able to split the edge into it.
+  if (BB->isEHPad())
+    return false;
+
   // If we have a xor as the branch input to this block, and we know that the
   // LHS or RHS of the xor in any predecessor is true/false, then we can clone
   // the condition into the predecessor and fix that value to true, saving some
   // logical ops on that path and encouraging other paths to simplify.
   //
   // This copies something like this:
   //
   //  BB:

test/Transforms/JumpThreading/pr27840.ll

+; RUN: opt -jump-threading -S < %s | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+declare void @helper()
+declare i32 @__gxx_personality_v0(...)
+
+
+define void @pr27840(i8* %call, i1 %A) personality i32(...)* @__gxx_personality_v0 {
+entry:
+  invoke void @helper()
+          to label %invoke.cont unwind label %lpad
+
+; Don't jump threading; we can't split the critical edge from entry to lpad.
+; CHECK-LABEL: @pr27840
+; CHECK: invoke
+; CHECK-NEXT: to label %invoke.cont unwind label %lpad
+
+invoke.cont:
+  invoke void @helper()
+          to label %nowhere unwind label %lpad
+
+lpad:
+  %b = phi i1 [ true, %invoke.cont ], [ false, %entry ]
+  landingpad { i8*, i32 }
+          cleanup
+  %xor = xor i1 %b, %A
+  br i1 %xor, label %nowhere, label %invoke.cont
+
+nowhere:
+  unreachable
+}