[PATCH] D106426: [FuncSpec] Support specialising recursive functions

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D106426#2913452 <https://reviews.llvm.org/D106426#2913452>, @SjoerdMeijer wrote:

> Looks like we agree after all! :-) Because I fully agree with this:
>
>> The key point I want to say is that we shouldn't specialize  recursiveFunc  so many times. If you feels like that it is OK, then how about setting  func-specialization-max-iters to 20 or 100?
>
> An early version of the function specialisation pass had a so called "aggressive mode". This meant that it was (still only) doing 10 iterations. That's what I envision what it could be roughly. But to be clear, I expect it to be a (very) low number. Simply because when we start to specialise more, it's going to cost more (compile-time). There's indeed no way around that, and that's why we agree on this. The cost of specialising more is not a problem with the implementation of this patch (but again, simply because of the resulting additional functions/instructions).
>
> About what the value of `-func-specialization-max-iters=` should be exactly, I don't know yet. I would like to defer that question to some point later, when I pick up enabling function specialisation. I would like to see if we can get it enabled first with `-func-specialization-max-iters=1`, and after if we can increase it, or if needs to remain opt-in.
>
> Like I said, I think we agree. That's why I would like to see first if we can finish this reviewer together, which I think would be a nicer result than getting other people involved at this point.

It looks like the different opinions come from different perspective. I prefer to look into the details.

Or let me ask the other question: do you think it matters that the recursive functions get specialized many times when `func-specialization-max-iters` increases? If yes, I think I could fix it in successive patches. It shouldn't be so hard.

Out of this, the overall patch looks good to me. If we can get consensus in above question, I would look into the implementation details.


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

https://reviews.llvm.org/D106426