[llvm-reduce] Don't delete arguments of intrinsics

[langston-barrett]

Summary

The argument reduction pass shouldn't remove arguments of
intrinsics, because the resulting module is ill-formed, and so
inherently uninteresting.

[aeubanks]

should we be skipping all intrinsics? intrinsic signatures should never change right?

and there are a couple comments that seem to indicate that we are only touching definitions, but it looks like we're also touching declarations

llvm/tools/llvm-reduce/deltas/ReduceArguments.cpp

10

drive by, this looks wrong

111–112

drive by, this looks wrong

[langston-barrett]

Generalize to all intrinsics

[langston-barrett]

should we be skipping all intrinsics? intrinsic signatures should never change right?

Good call, I've generalized this patch.

and there are a couple comments that seem to indicate that we are only touching definitions, but it looks like we're also touching declarations

Agreed.

[lebedev.ri]

I'm not okay with blankedly ignoring intrinsics.
Please can you show the malformed IR after dbginfo arg removal?

[aeubanks]

why wouldn't we always ignore intrinsics? their signature is always the same, it should be invalid to remove a parameter of an intrinsic

[langston-barrett]

Please can you show the malformed IR after dbginfo arg removal?

Here's the output of the testcase I added before this patch:

******************** TEST 'LLVM :: tools/llvm-reduce/remove-args-dbg-intrinsics.ll' FAILED ********************
Script:
--
: 'RUN: at line 3';   llvm-reduce --abort-on-invalid-reduction --delta-passes=arguments --test=/home/langston/code/llvm-project/build/bin/opt --test-arg=-verify --test-arg=-disable-output --output=/home/langston/code/llvm-project/build/test/tools/llvm-reduce/Output/remove-args-dbg-intrinsics.ll.tmp /home/langston/code/llvm-project/llvm/test/tools/llvm-reduce/remove-args-dbg-intrinsics.ll
: 'RUN: at line 4';   cat /home/langston/code/llvm-project/build/test/tools/llvm-reduce/Output/remove-args-dbg-intrinsics.ll.tmp | /home/langston/code/llvm-project/build/bin/FileCheck /home/langston/code/llvm-project/llvm/test/tools/llvm-reduce/remove-args-dbg-intrinsics.ll
--
Exit Code: 1

Command Output (stdout):
--
*** Reducing Arguments...
----------------------------
Param Index Reference:
  llvm.dbg.addr
	1: 
	2: 
	3: 
----------------------------
  llvm.dbg.declare
	4: 
	5: 
	6: 
----------------------------
  llvm.dbg.value
	7: 
	8: 
	9: 
----------------------------

--
Command Output (stderr):
--
Invalid user of intrinsic instruction!
void (metadata, metadata, metadata)* bitcast (void ()* @llvm.dbg.addr to void (metadata, metadata, metadata)*)
Invalid user of intrinsic instruction!
void (metadata, metadata, metadata)* bitcast (void ()* @llvm.dbg.declare to void (metadata, metadata, metadata)*)
Invalid user of intrinsic instruction!
void (metadata, metadata, metadata)* bitcast (void ()* @llvm.dbg.value to void (metadata, metadata, metadata)*)
Invalid reduction

--

********************
********************
Failed Tests (1):
  LLVM :: tools/llvm-reduce/remove-args-dbg-intrinsics.ll

If I run that same command without --abort-on-invalid-reduction, I just see many more WARNINGs, and the final module is identical to the input module. So IIUC, this patch doesn't result in different output from llvm-reduce, just fewer unnecessary *attempts* at reduction.

[aeubanks]

the title/summary needs to be updated

llvm/test/tools/llvm-reduce/remove-args-dbg-intrinsics.ll

4

I don't think this is the right test, llvm-reduce already checks that something is verified. The test should be checking that each of the three intrinsics is declared (ignoring the arguments). Look at the other tests in the file for more examples.

llvm/tools/llvm-reduce/deltas/ReduceArguments.cpp

47

F.isIntrinsic()

[lebedev.ri]

llvm/tools/llvm-reduce/deltas/ReduceArguments.cpp

47

While i could be okay with ignoring actual intriniscs,
i'm not sure why we need to ignore all functions starting with @llvm..

[aeubanks]

llvm/tools/llvm-reduce/deltas/ReduceArguments.cpp

47

https://llvm.org/docs/LangRef.html#intrinsic-functions
Intrinsic function names must all start with an “llvm.” prefix. This prefix is reserved in LLVM for intrinsic names; thus, function names may not begin with this prefix.

[langston-barrett]

Use FileCheck instead of opt as the interestingness test

[langston-barrett]

Use Function::isIntrinsic instead of Function::getIntrinsicID

[aeubanks]

could you also update the various comments to say that all functions, not just definitions, are touched?

llvm/test/tools/llvm-reduce/remove-args-dbg-intrinsics.ll

4

I think this is still not quite right.
The llvm-reduce test should only be checking for the names of the functions, whereas the final test should check that everything is there. Other tests do something similar (e.g. remove-dso-local.ll)

so something like

CHECK-INTERESTINGNESS: declare void @llvm.dbg.addr
CHECK-INTERESTINGNESS: declare void @llvm.dbg.declare
CHECK-INTERESTINGNESS: declare void @llvm.dbg.value

CHECK-FINAL: declare void @llvm.dbg.addr(metadata, metadata, metadata)
CHECK-FINAL: declare void @llvm.dbg.declare(metadata, metadata, metadata)
CHECK-FINAL: declare void @llvm.dbg.value(metadata, metadata, metadata)

[langston-barrett]

Improve tests (thanks for suggestions, @aeubanks)

[langston-barrett]

Fix comments about only working on defined functions

[aeubanks]

the displayed diff is only the latest diff

[langston-barrett]

Maybe fix uploaded diff (I'm new to Phabricator)

[langston-barrett]

Probably fix uploaded diff?

[aeubanks]

lgtm, thanks!

[langston-barrett]

lgtm, thanks!

Thanks for your patience and comments in the review! I don't have commit access, so I'll need some help merging this.

[aeubanks]

no problem, will do

llvm/test/tools/llvm-reduce/remove-args-dbg-intrinsics.ll

+; llvm-reduce shouldn't remove arguments of debug intrinsics, because the resulting module will be ill-formed.
+;
+; RUN: llvm-reduce --abort-on-invalid-reduction --delta-passes=arguments --test FileCheck --test-arg --check-prefixes=CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s --output %t
+; RUN: FileCheck --check-prefix=CHECK-FINAL %s < %t
+
+; CHECK-INTERESTINGNESS: declare void @llvm.dbg.addr
+; CHECK-FINAL: declare void @llvm.dbg.addr(metadata, metadata, metadata)
+declare void @llvm.dbg.addr(metadata, metadata, metadata)
+; CHECK-INTERESTINGNESS: declare void @llvm.dbg.declare
+; CHECK-FINAL: declare void @llvm.dbg.declare(metadata, metadata, metadata)
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+; CHECK-INTERESTINGNESS: declare void @llvm.dbg.value
+; CHECK-FINAL: declare void @llvm.dbg.value(metadata, metadata, metadata)
+declare void @llvm.dbg.value(metadata, metadata, metadata)

llvm/tools/llvm-reduce/deltas/ReduceArguments.cpp

 //===- ReduceArguments.cpp - Specialized Delta Pass -----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements a function which calls the Generic Delta pass in order
-// to reduce uninteresting Arguments from defined functions.
+// to reduce uninteresting Arguments from declared and defined functions.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ReduceArguments.h"
 #include "Delta.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/IR/Intrinsics.h"
 #include <set>
 #include <vector>
 
 using namespace llvm;
 
 /// Goes over OldF calls and replaces them with a call to NewF
 static void replaceFunctionCalls(Function &OldF, Function &NewF,
                                  const std::set<int> &ArgIndexesToKeep) {
   const auto &Users = OldF.users();
   for (auto I = Users.begin(), E = Users.end(); I != E; )
     if (auto *CI = dyn_cast<CallInst>(*I++)) {
       SmallVector<Value *, 8> Args;
       for (auto ArgI = CI->arg_begin(), E = CI->arg_end(); ArgI != E; ++ArgI)
         if (ArgIndexesToKeep.count(ArgI - CI->arg_begin()))
           Args.push_back(*ArgI);
 
       CallInst *NewCI = CallInst::Create(&NewF, Args);
       NewCI->setCallingConv(NewF.getCallingConv());
       if (!CI->use_empty())
         CI->replaceAllUsesWith(NewCI);
       ReplaceInstWithInst(CI, NewCI);
     }
 }
 
+/// Returns whether or not this function should be considered a candidate for
+/// argument removal. Currently, functions with no arguments and intrinsics are
+/// not considered. Intrinsics aren't considered because their signatures are
+/// fixed.
+static bool shouldRemoveArguments(const Function &F) {
+  return !F.arg_empty() && !F.isIntrinsic();
+}
+
 /// Removes out-of-chunk arguments from functions, and modifies their calls
 /// accordingly. It also removes allocations of out-of-chunk arguments.
 static void extractArgumentsFromModule(std::vector<Chunk> ChunksToKeep,
                                        Module *Program) {
   Oracle O(ChunksToKeep);
 
   std::set<Argument *> ArgsToKeep;
   std::vector<Function *> Funcs;
   // Get inside-chunk arguments, as well as their parent function
   for (auto &F : *Program)
-    if (!F.arg_empty()) {
+    if (shouldRemoveArguments(F)) {
       Funcs.push_back(&F);
       for (auto &A : F.args())
         if (O.shouldKeep())
           ArgsToKeep.insert(&A);
     }
 
   for (auto *F : Funcs) {
     ValueToValueMapTy VMap;
for (auto *F : Funcs) {
     replaceFunctionCalls(*F, *ClonedFunc, ArgIndexesToKeep);
     // Rename Cloned Function to Old's name
     std::string FName = std::string(F->getName());
     F->replaceAllUsesWith(ConstantExpr::getBitCast(ClonedFunc, F->getType()));
     F->eraseFromParent();
     ClonedFunc->setName(FName);
   }
 }
 
-/// Counts the amount of arguments in non-declaration functions and prints their
-/// respective name, index, and parent function name
+/// Counts the amount of arguments in functions and prints their respective
+/// name, index, and parent function name
 static int countArguments(Module *Program) {
   // TODO: Silence index with --quiet flag
   outs() << "----------------------------\n";
   outs() << "Param Index Reference:\n";
   int ArgsCount = 0;
   for (auto &F : *Program)
-    if (!F.arg_empty()) {
+    if (shouldRemoveArguments(F)) {
       outs() << "  " << F.getName() << "\n";
       for (auto &A : F.args())
         outs() << "\t" << ++ArgsCount << ": " << A.getName() << "\n";
 
       outs() << "----------------------------\n";
     }
 
   return ArgsCount;
 }
 
 void llvm::reduceArgumentsDeltaPass(TestRunner &Test) {
   outs() << "*** Reducing Arguments...\n";
   int ArgCount = countArguments(Test.getProgram());
   runDeltaPass(Test, ArgCount, extractArgumentsFromModule);
 }