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

Wed Oct 13 07:58:28 PDT 2021

SjoerdMeijer added a comment.

In D111661#3061016 <https://reviews.llvm.org/D111661#3061016>, @duck-37 wrote:

> In D111661#3060651 <https://reviews.llvm.org/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 are a no brainer), but like I said at least 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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111661/new/

https://reviews.llvm.org/D111661