[llvm-reduce] Fix incorrect indices in argument reduction pass

[arichardson]

Summary

The function extractArgumentsFromModule() was passing a one-based index to,
but replaceFunctionCalls() was expecting a zero-based argument index. This
resulted in assertion errors when reducing function call arguments with
different types. Additionally, the

[lebedev.ri]

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

84–87

Let's take this one step further, shall we?

std::set<int> ArgIndexesToKeep;
for (auto &Arg : enumerate(F->args()))
  if (ArgsToKeep.count(&Arg.value()))
    ArgIndexesToKeep.insert(Arg.index());

[arichardson]

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

84–87

Yes, much better! I realized just after submitting this patch that the index is not always being incremented so my non-minimal test case still crashed. Will add a better test case.

[arichardson]

Fix missing increment using @lebedev.ri's suggestion
Improve test case to crash without this change.

[lebedev.ri]

LG, thank you.

llvm/test/Reduce/remove-args-2.ll

5

I guess i will indeed have to cleanup all the existing tests :S

[dblaikie]

llvm/test/Reduce/remove-args-2.ll

5–6

Sorry I've only been watching this work superficially - but it looked like some other patches were moving towards using FileCheck directly (as the test program) for new llvm-reduce tests? Could that be done here too?

[arichardson]

llvm/test/Reduce/remove-args-2.ll

5–6

Yeah that would definitely work. I didn't do it here since the test I used as a baseline didn't and I was able to reuse the same script.

I can commit a follow-up change to use Filecheck for this test unless @lebedev.ri already has a change to clean up the existing tests.

[lebedev.ri]

llvm/test/Reduce/remove-args-2.ll

5–6

I can commit a follow-up change to use Filecheck for this test

Please feel free to. See latest tests i've added for examples.

[arichardson]

llvm/test/Reduce/remove-args-2.ll

5–6

Fixed in rGf5d4a58b4a2621674de97ea3f796687eb0810c7f

llvm/test/Reduce/remove-args-2.ll

+; Test that llvm-reduce can remove uninteresting function arguments from function definitions as well as their calls.
+; This test checks that functions with different argument types are handled correctly
+;
+; RUN: llvm-reduce --test %python --test-arg %p/Inputs/remove-args.py %s -o %t
+; RUN: cat %t | FileCheck -implicit-check-not=uninteresting %s
+
+%struct.foo = type { %struct.foo*, i32, i32, i8* }
+
+define dso_local void @bar() {
+entry:
+  ; CHECK: call void @interesting(%struct.foo* null)
+  call void @interesting(%struct.foo* null, i8* null)
+  ret void
+}
+
+; CHECK: define internal void @interesting(%struct.foo* %interesting) {
+define internal void @interesting(%struct.foo* %interesting, i8* %uninteresting) {
+entry:
+  ret void
+}

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

for (Value *V : InstToDelete) {
       I->eraseFromParent();
     }
 
     // No arguments to reduce
     if (VMap.empty())
       continue;
 
     std::set<int> ArgIndexesToKeep;
     int ArgI = 0;
     for (auto &Arg : F->args())
       if (ArgsToKeep.count(&Arg))
-        ArgIndexesToKeep.insert(++ArgI);
+        ArgIndexesToKeep.insert(ArgI++);
 
     auto *ClonedFunc = CloneFunction(F, VMap);
     // In order to preserve function order, we move Clone after old Function
     ClonedFunc->removeFromParent();
     Program->getFunctionList().insertAfter(F->getIterator(), ClonedFunc);
 
     replaceFunctionCalls(*F, *ClonedFunc, ArgIndexesToKeep);
     // Rename Cloned Function to Old's name