[FuncSpec] Only visit uses of the instruction instead of every constant use.

[duck-37]

Summary

This drastically reduces the time this pass takes on larger modules (15 minutes to 30 seconds on a 500MB LTO module I'm working with)

[SjoerdMeijer]

Thanks, that sounds like a really good improvement!

I just wanted to quickly check your testing strategy. Problem is function specialisation isn't enabled by default, so isn't covered by the upstream buildbots and tests. I shot myself in the foot recently by introducing a performance regression that I noticed later and then took me more time to fix (I am tracking performance now with function specialisation enabled).
I usually hack the compiler in Transforms/IPO/PassManagerBuilder.cpp:175 as I find that easier to pass flags in different build systems and change cl::init(false) into true, and then run some code: a stage2 build would be good, and the llvm test suite too, these would be the minimum and function specialisation should trigger in both. And sorry if you're doing this already, but just wanted to check. If you can it would be good if you run SPEC too and see that the MCF score is unaffected (with LTO that is, it should trigger on MCF so if it doesn't anymore we have a problem :) ).
But I think I can quickly run SPEC today, so will do that and report back here.

[ChuanqiXu]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

304

It looks like this line should be included.

[duck-37]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

304

This is supplemented by the replaceUsesOfWith in the patch. However, when looking over this, I realized that the way I submitted this is just plain wrong.

[duck-37]

Visit with SCCPSolver after replacing users.

[duck-37]

Make sure not to diff off of a local branch instead of main.

[ChuanqiXu]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

304

Thanks for updating this. Now it looks more reasonable. But I am still confused. After line 304, V shouldn't have any uses, right? So how could we replace uses for V on line 308?

[duck-37]

In D111661#3060651, @SjoerdMeijer wrote:

Thanks, that sounds like a really good improvement!

I just wanted to quickly check your testing strategy. Problem is function specialisation isn't enabled by default, so isn't covered by the upstream buildbots and tests. I shot myself in the foot recently by introducing a performance regression that I noticed later and then took me more time to fix (I am tracking performance now with function specialisation enabled).
I usually hack the compiler in Transforms/IPO/PassManagerBuilder.cpp:175 as I find that easier to pass flags in different build systems and change cl::init(false) into true, and then run some code: a stage2 build would be good, and the llvm test suite too, these would be the minimum and function specialisation should trigger in both. And sorry if you're doing this already, but just wanted to check. If you can it would be good if you run SPEC too and see that the MCF score is unaffected (with LTO that is, it should trigger on MCF so if it doesn't anymore we have a problem :) ).
But I think I can quickly run SPEC today, so will do that and report back here.

My testing "strategy" at the moment is essentially just throwing this insane LTO module at it and seeing whether it breaks or not. Problem is I have a lot of weird patches on the pass at the moment so it's somewhat difficult to isolate individual things to upstream. Will try running tests later.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

304

Oh, I see what you mean now. You're right, this should be in the diff too.

[duck-37]

Make sure replaceAllUsesWith is also removed.

[SjoerdMeijer]

In D111661#3061016, @duck-37 wrote:

In D111661#3060651, @SjoerdMeijer wrote:

Thanks, that sounds like a really good improvement!

I just wanted to quickly check your testing strategy. Problem is function specialisation isn't enabled by default, so isn't covered by the upstream buildbots and tests. I shot myself in the foot recently by introducing a performance regression that I noticed later and then took me more time to fix (I am tracking performance now with function specialisation enabled).
I usually hack the compiler in Transforms/IPO/PassManagerBuilder.cpp:175 as I find that easier to pass flags in different build systems and change cl::init(false) into true, and then run some code: a stage2 build would be good, and the llvm test suite too, these would be the minimum and function specialisation should trigger in both. And sorry if you're doing this already, but just wanted to check. If you can it would be good if you run SPEC too and see that the MCF score is unaffected (with LTO that is, it should trigger on MCF so if it doesn't anymore we have a problem :) ).
But I think I can quickly run SPEC today, so will do that and report back here.

My testing "strategy" at the moment is essentially just throwing this insane LTO module at it and seeing whether it breaks or not. Problem is I have a lot of weird patches on the pass at the moment so it's somewhat difficult to isolate individual things to upstream. Will try running tests later.

That's a good start! :-) But yeah, please try to test more (the llvm regr tests are a no brainer), but like I said at least the llvm test suite and a stage2 build would be good.
I've run SPEC and that seems unaffected so is good.

I haven't looked much into this change itself to be honest (thanks @ChuanqiXu ), but the different iterations of this patch makes me wonder if some of this could or should have been caught by regression tests. Are we missing some coverage there?

[duck-37]

In D111661#3061399, @SjoerdMeijer wrote:

In D111661#3061016, @duck-37 wrote:

In D111661#3060651, @SjoerdMeijer wrote:

Thanks, that sounds like a really good improvement!

I just wanted to quickly check your testing strategy. Problem is function specialisation isn't enabled by default, so isn't covered by the upstream buildbots and tests. I shot myself in the foot recently by introducing a performance regression that I noticed later and then took me more time to fix (I am tracking performance now with function specialisation enabled).
I usually hack the compiler in Transforms/IPO/PassManagerBuilder.cpp:175 as I find that easier to pass flags in different build systems and change cl::init(false) into true, and then run some code: a stage2 build would be good, and the llvm test suite too, these would be the minimum and function specialisation should trigger in both. And sorry if you're doing this already, but just wanted to check. If you can it would be good if you run SPEC too and see that the MCF score is unaffected (with LTO that is, it should trigger on MCF so if it doesn't anymore we have a problem :) ).
But I think I can quickly run SPEC today, so will do that and report back here.

My testing "strategy" at the moment is essentially just throwing this insane LTO module at it and seeing whether it breaks or not. Problem is I have a lot of weird patches on the pass at the moment so it's somewhat difficult to isolate individual things to upstream. Will try running tests later.

That's a good start! :-) But yeah, please try to test more (the llvm regr tests are a no brainer), but like I said at least the llvm test suite and a stage2 build would be good.
I've run SPEC and that seems unaffected so is good.

I haven't looked much into this change itself to be honest (thanks @ChuanqiXu ), but the different iterations of this patch makes me wonder if some of this could or should have been caught by regression tests. Are we missing some coverage there?

That's a good point, I can try to create a test case that would break the previous version of this patch.
But now that I think about it, is this necessary at all? We're not changing the value to something completely different, and the solver already knows it's constant which means this information has already been backpropagated to the user list.
This would also explain why the previous patches didn't break any tests; this code might essentially be a no-op.

Also further supported by the fact that SCCP doesn't do this either.

[ChuanqiXu]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

306–311

This comment may be confusing. We could understand this comment due to we are visiting this diff page. But other readers may be confused about this since they lost the context.

[duck-37]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

306–311

That's true, but I'm thinking about holding off on this patch because I think this entire code block might not be needed.

[duck-37]

Note: abandoning this due to concerns mentioned above. Will run some more testing with this entire block removed.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

if (!V->getType()->isSingleValueType() || isa<CallBase>(V) ||
         V->user_empty())
       return false;
 
     const ValueLatticeElement &IV = Solver.getLatticeValueFor(V);
     if (isOverdefined(IV))
       return false;
     auto *Const =
         isConstant(IV) ? Solver.getConstant(IV) : UndefValue::get(V->getType());
     V->replaceAllUsesWith(Const);
 
-    for (auto *U : Const->users())
-      if (auto *I = dyn_cast<Instruction>(U))
+    // Using replaceUsesWithIf is probably the easiest way to do this without
+    // duplicating code.
+    SmallVector<Instruction *, 8> ReplacedUsers;
+    V->replaceUsesWithIf(Const, [&](auto &U) {
+      if (auto *I = dyn_cast<Instruction>(U.getUser()))
         if (Solver.isBlockExecutable(I->getParent()))
-          Solver.visit(I);
+          ReplacedUsers.push_back(I);
+      return true;
+    });
+    for (auto *I : ReplacedUsers)
 
     // Remove the instruction from Block and Solver.
     if (auto *I = dyn_cast<Instruction>(V)) {
       if (I->isSafeToRemove()) {
         I->eraseFromParent();
         Solver.removeLatticeValueFor(I);
       }
     }