[Attributor] Add helper to change an instruction to `unreachable` inst

[uenoku]

Summary

Calling changeToUnreachable in manifest from different places might cause really unpredictable problems. As other deleting functions are doing, we need to change these instructions after all manifest.

[jdoerfert]

This makes sense and LGTM.

FWIW. We have to go over the entire "rewrite after manifest" stuff soon.

[uenoku]

In D71910#1796777, @jdoerfert wrote:

This makes sense and LGTM.

FWIW. We have to go over the entire "rewrite after manifest" stuff soon.

I think so too.

[baziotis]

llvm/include/llvm/Transforms/IPO/Attributor.h

1040–1041

How about a SmallVector here? I think it makes sense since we never lookup values on this set (which is I guess the primary reason to make a set), it should have amortized O(1) and it is easier to loop over.

1040–1041

Oh my bad, I just saw it was committed.

[uenoku]

llvm/include/llvm/Transforms/IPO/Attributor.h

1040–1041

I think SmallVector isn't a set. We don't want to change the same instruction to unreachable twice so SmallPtrSet seems good to me.

[baziotis]

llvm/include/llvm/Transforms/IPO/Attributor.h

1040–1041

Yes, that's a safer way.

llvm/include/llvm/Transforms/IPO/Attributor.h

if (V && (V->stripPointerCasts() == NV.stripPointerCasts() ||
               isa_and_nonnull<UndefValue>(V)))
       return false;
     assert((!V || V == &NV || isa<UndefValue>(NV)) &&
            "Use was registered twice for replacement with different values!");
     V = &NV;
     return true;
   }
 
+  /// Record that \p I is to be replaced with `unreachable` after information
+  /// was manifested.
+  void changeToUnreachableAfterManifest(Instruction *I) {
+    ToBeChangedToUnreachableInsts.insert(I);
+  }
+
   /// Record that \p I is deleted after information was manifested. This also
   /// triggers deletion of trivially dead istructions.
   void deleteAfterManifest(Instruction &I) { ToBeDeletedInsts.insert(&I); }
 
   /// Record that \p BB is deleted after information was manifested. This also
   /// triggers deletion of trivially dead istructions.
   void deleteAfterManifest(BasicBlock &BB) { ToBeDeletedBlocks.insert(&BB); }
 
private:
 
   /// A set to remember the functions we already assume to be live and visited.
   DenseSet<const Function *> VisitedFunctions;
 
   /// Uses we replace with a new value after manifest is done. We will remove
   /// then trivially dead instructions as well.
   DenseMap<Use *, Value *> ToBeChangedUses;
 
+  /// Instructions we replace with `unreachable` insts after manifest is done.
+  SmallPtrSet<Instruction *, 8> ToBeChangedToUnreachableInsts;
+
   /// Functions, blocks, and instructions we delete after manifest is done.
   ///
   ///{
   SmallPtrSet<Function *, 8> ToBeDeletedFunctions;
   SmallPtrSet<BasicBlock *, 8> ToBeDeletedBlocks;
   SmallPtrSet<Instruction *, 8> ToBeDeletedInsts;
   ///}
 };

llvm/lib/Transforms/IPO/Attributor.cpp

struct AAUndefinedBehaviorImpl : public AAUndefinedBehavior {
   bool isAssumedToCauseUB(Instruction *I) const override {
     return UBMemAccessInsts.count(I);
   }
 
   ChangeStatus manifest(Attributor &A) override {
     if (!UBMemAccessInsts.size())
       return ChangeStatus::UNCHANGED;
     for (Instruction *I : UBMemAccessInsts)
-      changeToUnreachable(I, /* UseLLVMTrap */ false);
+      A.changeToUnreachableAfterManifest(I);
     return ChangeStatus::CHANGED;
   }
 
   /// See AbstractAttribute::getAsStr()
   const std::string getAsStr() const override {
     return getAssumed() ? "undefined-behavior" : "no-ub";
   }
 
for (const Instruction *DeadEndI : KnownDeadEnds) {
         }
       }
 
       if (isa_and_nonnull<UnreachableInst>(SplitPos))
         continue;
 
       BB = SplitPos->getParent();
       SplitBlock(BB, SplitPos);
-      changeToUnreachable(BB->getTerminator(), /* UseLLVMTrap */ false);
+      A.changeToUnreachableAfterManifest(BB->getTerminator());
       HasChanged = ChangeStatus::CHANGED;
     }
 
     for (BasicBlock &BB : F)
       if (!AssumedLiveBlocks.count(&BB))
         A.deleteAfterManifest(BB);
 
     return HasChanged;
// Delete stuff at the end to avoid invalid references and a nice order.
     LLVM_DEBUG(dbgs() << "\n[Attributor] Delete at least "
                       << ToBeDeletedFunctions.size() << " functions and "
                       << ToBeDeletedBlocks.size() << " blocks and "
                       << ToBeDeletedInsts.size() << " instructions and "
                       << ToBeChangedUses.size() << " uses\n");
 
     SmallVector<Instruction *, 32> DeadInsts;
     SmallVector<Instruction *, 32> TerminatorsToFold;
-    SmallVector<Instruction *, 32> UnreachablesToInsert;
 
     for (auto &It : ToBeChangedUses) {
       Use *U = It.first;
       Value *NewV = It.second;
       Value *OldV = U->get();
       LLVM_DEBUG(dbgs() << "Use " << *NewV << " in " << *U->getUser()
                         << " instead of " << *OldV << "\n");
       U->set(NewV);
       if (Instruction *I = dyn_cast<Instruction>(OldV))
         if (!isa<PHINode>(I) && !ToBeDeletedInsts.count(I) &&
             isInstructionTriviallyDead(I)) {
           DeadInsts.push_back(I);
         }
       if (isa<Constant>(NewV) && isa<BranchInst>(U->getUser())) {
         Instruction *UserI = cast<Instruction>(U->getUser());
         if (isa<UndefValue>(NewV)) {
-          UnreachablesToInsert.push_back(UserI);
+          ToBeChangedToUnreachableInsts.insert(UserI);
         } else {
           TerminatorsToFold.push_back(UserI);
         }
       }
     }
-    for (Instruction *I : UnreachablesToInsert)
+    for (Instruction *I : ToBeChangedToUnreachableInsts)
       changeToUnreachable(I, /* UseLLVMTrap */ false);
     for (Instruction *I : TerminatorsToFold)
       ConstantFoldTerminator(I->getParent());
 
     for (Instruction *I : ToBeDeletedInsts) {
       I->replaceAllUsesWith(UndefValue::get(I->getType()));
       if (!isa<PHINode>(I) && isInstructionTriviallyDead(I))
         DeadInsts.push_back(I);