[bugpoint] Also replace declarations with null.

[fhahn]

Summary

Unless I am missing something, we can also replace calls to declarations
with null, similar to the way we handle regular functions. This helps to
reduce a few more cases and also reduces the number of globals in the
module we are working on.

[reames]

Calling null is (I think) full out UB. This may be a more hostile reduction strategy than is warranted. It's likely to result in extremely unstable reductions.

An alternative would be to pick a canonical callee (non-null declaration) and map all calls of a given signature to the same decl.

Also, I'm not opposed to this version if you really want, just pointing out a possible downside.

[fhahn]

In D74677#1891680, @reames wrote:

Calling null is (I think) full out UB. This may be a more hostile reduction strategy than is warranted. It's likely to result in extremely unstable reductions.

An alternative would be to pick a canonical callee (non-null declaration) and map all calls of a given signature to the same decl.

My original intention with this patch was to just extend the existing behavior to declarations as well, but I agree using null is not ideal. I'll give your suggestion a try!

[reames]

In D74677#1892136, @fhahn wrote:

In D74677#1891680, @reames wrote:

Calling null is (I think) full out UB. This may be a more hostile reduction strategy than is warranted. It's likely to result in extremely unstable reductions.

An alternative would be to pick a canonical callee (non-null declaration) and map all calls of a given signature to the same decl.

My original intention with this patch was to just extend the existing behavior to declarations as well, but I agree using null is not ideal. I'll give your suggestion a try!

I possibly didn't read far enough. Are you saying we will replace the target of a call to a defined function w/null, but not for a call to a declaration? If so, then yes, simply being consistent is better than nothing. If that's the case, LGTM. :)

[fhahn]

In D74677#1892434, @reames wrote:

In D74677#1892136, @fhahn wrote:

In D74677#1891680, @reames wrote:

Calling null is (I think) full out UB. This may be a more hostile reduction strategy than is warranted. It's likely to result in extremely unstable reductions.

An alternative would be to pick a canonical callee (non-null declaration) and map all calls of a given signature to the same decl.

My original intention with this patch was to just extend the existing behavior to declarations as well, but I agree using null is not ideal. I'll give your suggestion a try!

I possibly didn't read far enough. Are you saying we will replace the target of a call to a defined function w/null, but not for a call to a declaration? If so, then yes, simply being consistent is better than nothing. If that's the case, LGTM. :)

Yes that's the case at the moment IIUC. This patch should just extend the existing behavior to also include declarations.

[fhahn]

Bugpoint is loosing relevance, I won't be able to follow up on this anytime soon.

llvm/test/BugPoint/reduce-declarations.ll

+; REQUIRES: plugins
+; RUN: bugpoint -load %llvmshlibdir/BugpointPasses%shlibext %s -output-prefix %t -bugpoint-crashname -silence-passes > /dev/null
+; RUN: llvm-dis %t-reduced-simplified.bc -o - | FileCheck --check-prefix=NONUKE %s
+;
+; RUN: bugpoint -load %llvmshlibdir/BugpointPasses%shlibext %s -output-prefix %t -bugpoint-crashname -silence-passes -replace-funcs-with-null > /dev/null
+; RUN: llvm-dis %t-reduced-simplified.bc -o - | FileCheck --check-prefix=NUKE %s
+
+declare i32 @foo()
+declare i32 @crashing()
+declare dso_local i32 @bar()
+
+; NONUKE-NOT: @foo
+; NONUKE: declare void @crashing
+; NONUKE-NOT: @foo
+; NONUKE-LABEL: define void @test(
+; NONUKE-NEXT:    call void @crashing()
+; NONUKE-NEXT:    ret void
+
+; NUKE-NOT: @foo
+; NUKE-NOT: @crashing
+; NUKE-NOT: @foo
+; NUKE-LABEL: define void @test(
+; NUKE-NEXT:    %v2 = call i32 null()
+; NUKE-NEXT:    ret void
+
+define i32 @test(i32* %a, i8* %b) {
+    %v1 = call i32 @foo()
+    %v2 = call i32 @crashing()
+    %t = add i32 %v1, %v2
+    ret i32 %t
+}

llvm/tools/bugpoint-passes/TestPasses.cpp

 };
 } // namespace
 
 char CrashOnMetadata::ID = 0;
 static RegisterPass<CrashOnMetadata>
     C("bugpoint-crashmetadata",
       "BugPoint Test Pass - Intentionally crash on "
       "fabs calls with fpmath metadata and an fadd as argument");
+
+namespace {
+class CrashOnCall : public FunctionPass {
+public:
+  static char ID; // Pass ID, replacement for typeid
+  CrashOnCall() : FunctionPass(ID) {}
+
+private:
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.setPreservesAll();
+  }
+
+  // Crash on any instructions using values name 'crashing'.
+  bool runOnFunction(Function &F) override {
+    for (Instruction &I : instructions(F))
+      if (isa<CallInst>(&I))
+        abort();
+    return false;
+  }
+};
+} // namespace
+
+char CrashOnCall::ID = 0;
+static RegisterPass<CrashOnCall>
+    D("bugpoint-crashname",
+      "BugPoint Test Pass - Intentionally crash on calls;");

llvm/tools/bugpoint/CrashDebugger.cpp

bool ReduceCrashingFunctions::TestFuncs(std::vector<Function *> &Funcs) {
   }
 
   outs() << "Checking for crash with only these functions: ";
   PrintFunctionList(Funcs);
   outs() << ": ";
   if (!ReplaceFuncsWithNull) {
     // Loop over and delete any functions which we aren't supposed to be playing
     // with...
-    for (Function &I : *M)
-      if (!I.isDeclaration() && !Functions.count(&I))
+    for (Function &I : make_early_inc_range(*M)) {
+      if (Functions.count(&I))
+        continue;
+      if (!I.isDeclaration()) {
         DeleteFunctionBody(&I);
+      }
+    }
   } else {
     std::vector<GlobalValue *> ToRemove;
     // First, remove aliases to functions we're about to purge.
     for (GlobalAlias &Alias : M->aliases()) {
       GlobalObject *Root = Alias.getBaseObject();
       Function *F = dyn_cast_or_null<Function>(Root);
       if (F) {
         if (Functions.count(F))
for (GlobalAlias &Alias : M->aliases()) {
 
       PointerType *Ty = cast<PointerType>(Alias.getType());
       Constant *Replacement = ConstantPointerNull::get(Ty);
       Alias.replaceAllUsesWith(Replacement);
       ToRemove.push_back(&Alias);
     }
 
     for (Function &I : *M) {
-      if (!I.isDeclaration() && !Functions.count(&I)) {
+      if (!Functions.count(&I)) {
         PointerType *Ty = cast<PointerType>(I.getType());
         Constant *Replacement = ConstantPointerNull::get(Ty);
         I.replaceAllUsesWith(Replacement);
         ToRemove.push_back(&I);
       }
     }
 
     for (auto *F : ToRemove) {
static Error DebugACrash(BugDriver &BD, BugTester TestFn) {
   // in the program...
   if (!NoGlobalRM)
     if (Error E = ReduceGlobalInitializers(BD, TestFn))
       return E;
 
   // Now try to reduce the number of functions in the module to something small.
   std::vector<Function *> Functions;
   for (Function &F : BD.getProgram())
-    if (!F.isDeclaration())
       Functions.push_back(&F);
 
   if (Functions.size() > 1 && !BugpointIsInterrupted) {
     outs() << "\n*** Attempting to reduce the number of functions "
               "in the testcase\n";
 
     unsigned OldSize = Functions.size();
     Expected<bool> Result =