[PredicateInfo] Do not process unreachable operands.

[twoh]

Summary

We should excluded unreachable operands from processing as their DFS visitation order is undefined. When renameUses function sorts OpsToRename (https://fburl.com/d2wubn60), the comparator assumes that the parent block of the operand has a corresponding dominator tree node. This is not the case for unreachable operands and crashes the compiler.

[twoh]

Friendly ping. Thanks!

[twoh]

Friendly ping again. This is a fix for a compiler crash.

[efriedma]

Testcase?

For any non-PHI instruction that is reachable, all of its operands must also be reachable. We can use this to reduce the number of places we need to check for reachability. I'm pretty sure the new checks in processSwitch and processBranch are useless: iterating over the domtree will not visit unreachable blocks. And we should probably add a check to PredicateInfo::buildPredicateInfo to avoid processing assumes in unreachable code, rather than checking the reachability of the operands of an assume.

[twoh]

Thank you for your comments @efriedma! I addressed your comments and added a test case. The test segfaults clang without proposed change.

[efriedma]

LGTM with one minor comment.

llvm/lib/Transforms/Utils/PredicateInfo.cpp

495

It would be more clear to use isReachableFromEntry (which does the same thing, but it has a better name).

[twoh]

Use isReachableFromEntry. Thank you for the suggestion!

llvm/lib/Transforms/Utils/PredicateInfo.cpp

if (m_c_And(m_Cmp(Pred, m_Value(), m_Value()),
 
     ConditionsToProcess.push_back(Operand);
   }
   for (auto Cond : ConditionsToProcess) {
     if (auto *Cmp = dyn_cast<CmpInst>(Cond)) {
       collectCmpOps(Cmp, CmpOperands);
       // Now add our copy infos for our operands
       for (auto *Op : CmpOperands) {
+        // Unreachable Ops are out of interest.
+        if (Instruction *I = dyn_cast<Instruction>(Op))
+          if (!DT.getNode(I->getParent()))
+            continue;
+
         auto *PA = new PredicateAssume(Op, II, Cmp);
         addInfoFor(OpsToRename, Op, PA);
       }
       CmpOperands.clear();
     } else if (auto *BinOp = dyn_cast<BinaryOperator>(Cond)) {
       // Otherwise, it should be an AND.
       assert(BinOp->getOpcode() == Instruction::And &&
              "Should have been an AND");
+      // Unreachable Ops are out of interest.
+      if (!DT.getNode(BinOp->getParent()))
+        continue;
+
       auto *PA = new PredicateAssume(BinOp, II, BinOp);
       addInfoFor(OpsToRename, BinOp, PA);
     } else {
       llvm_unreachable("Unknown type of condition");
     }
   }
 }
 
 // Process a block terminating branch, and place relevant operations to be
 // renamed into OpsToRename.
 void PredicateInfo::processBranch(BranchInst *BI, BasicBlock *BranchBB,
                                   SmallPtrSetImpl<Value *> &OpsToRename) {
   BasicBlock *FirstBB = BI->getSuccessor(0);
   BasicBlock *SecondBB = BI->getSuccessor(1);
   SmallVector<BasicBlock *, 2> SuccsToProcess;
   SuccsToProcess.push_back(FirstBB);
   SuccsToProcess.push_back(SecondBB);
   SmallVector<Value *, 2> ConditionsToProcess;
 
   auto InsertHelper = [&](Value *Op, bool isAnd, bool isOr, Value *Cond) {
+    // Unreachable Ops are out of interest.
+    if (Instruction *I = dyn_cast<Instruction>(Op))
+      if (!DT.getNode(I->getParent()))
+        return;
+
     for (auto *Succ : SuccsToProcess) {
       // Don't try to insert on a self-edge. This is mainly because we will
       // eliminate during renaming anyway.
       if (Succ == BranchBB)
         continue;
       bool TakenEdge = (Succ == FirstBB);
       // For and, only insert on the true edge
       // For or, only insert on the false edge

 // Process a block terminating switch, and place relevant operations to be
 // renamed into OpsToRename.
 void PredicateInfo::processSwitch(SwitchInst *SI, BasicBlock *BranchBB,
                                   SmallPtrSetImpl<Value *> &OpsToRename) {
   Value *Op = SI->getCondition();
   if ((!isa<Instruction>(Op) && !isa<Argument>(Op)) || Op->hasOneUse())
     return;
 
+  // Unreachable Ops are out of interest.
+  if (Instruction *I = dyn_cast<Instruction>(Op))
+    if (!DT.getNode(I->getParent()))
+      return;
+
   // Remember how many outgoing edges there are to every successor.
   SmallDenseMap<BasicBlock *, unsigned, 16> SwitchEdges;
   for (unsigned i = 0, e = SI->getNumSuccessors(); i != e; ++i) {
     BasicBlock *TargetBlock = SI->getSuccessor(i);
     ++SwitchEdges[TargetBlock];
   }
 
   // Now propagate info for each case value
if (auto *BI = dyn_cast<BranchInst>(BranchBB->getTerminator())) {
         continue;
       processBranch(BI, BranchBB, OpsToRename);
     } else if (auto *SI = dyn_cast<SwitchInst>(BranchBB->getTerminator())) {
       processSwitch(SI, BranchBB, OpsToRename);
     }
   }
   for (auto &Assume : AC.assumptions()) {
     if (auto *II = dyn_cast_or_null<IntrinsicInst>(Assume))
       processAssume(II, II->getParent(), OpsToRename);
   }
   // Now rename all our operations.
   renameUses(OpsToRename);
 }
 
 // Create a ssa_copy declaration with custom mangling, because
 // Intrinsic::getDeclaration does not handle overloaded unnamed types properly:
 // all unnamed types get mangled to the same string. We use the pointer