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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

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

>> If we run bin/opt -function-specialization -func-specialization-max-iters=10  -S %s, we could find that recursiveFunc would get specialized 18 times.
>
> This is not not true. It will get specialised 10 times.

Sorry, I count the suffix number directly. But it still doesn't make sense to me. `recursiveFunc ` shouldn't be specialized so many times. I guess we would be in consensus in this.

>> Then I run bin/opt -function-specialization -func-specialization-max-iters=10 -inline -instcombine -simplifycfg -S %s, I got:
>> <SNIP>
>> hmmmm, I guess it must not be your expected code, right?
>
> Hmmm, sorry, this is exactly what I expect. Look at the generated code:
>
>   _main:                                  ; @main
>         stp     x20, x19, [sp, #-32]!           ; 16-byte Folded Spill
>         stp     x29, x30, [sp, #16]             ; 16-byte Folded Spill
>         adrp    x19, _funcspec.arg.17 at PAGE
>         add     x19, x19, _funcspec.arg.17 at PAGEOFF
>   LBB1_1:
>         mov     w0, #1
>         bl      _print_val
>   LBB1_2:
>         mov     w0, #2
>         bl      _print_val
>   LBB1_3:
>         mov     w0, #3
>         bl      _print_val
>   LBB1_4:
>         mov     w0, #4
>         bl      _print_val
>   LBB1_5:
>         mov     w0, #5
>         bl      _print_val
>   LBB1_6:
>         mov     w0, #6
>         bl      _print_val
>   LBB1_7:
>         mov     w0, #7
>         bl      _print_val
>   LBB1_8:
>         mov     w0, #8
>         bl      _print_val
>   LBB1_9:
>         mov     w0, #9
>         bl      _print_val
>         mov     x0, x19
>         bl      _recursiveFunc
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_9
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_9
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_9
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_9
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_8
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_8
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_8
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_8
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_8
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_7
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_7
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_7
>         bl      _exit_cond
>         tbnz    w0, #0, LBB1_7
>         bl      _exit_cond
>         <SNIP: a few more>
>
> That seem fairly optimal to me, so don't know why we wouldn't be doing this, or what we need to differently.

hmmmm, the generated code looks not good enough to me. There are too many redundancies in the main function.
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? I feel like it should be easy to agree that there should be a limit for recursive functions.

>> I think all of them would be affected. The impact on code-size is obvious. For the compile-time, it would take a long time to compile if we set the argument  func-specialization-max-iters to 2000.
>
> I am sorry, but this is an unreasonable number. There is no point in discussing such a big number, because the default will never be even close to that, and if you do want to set it to very high, then that's opt-in and your responsibility.

I just want to note that the compile time would increase significantly with the parameter `func-specialization-max-iters`
For the above example, I got the following results by using opt to compile:
(compile option is `-function-specialization -func-specialization-max-iters=100 -inline -instcombine -simplifycfg`)

| func-specialization-max-iters | compile time (seconds) |
| ----------------------------- | ---------------------- |
| 1                             | 0.0096                 |
| 10                            | 0.1253                 |
| 20                            | 0.4127                 |
| 50                            | 2.4648                 |
| 100                           | 5.9211                 |
|

hmm, it looks not good.

---

Since the discuss involves the different perspective to review a patch. So it may be helpful to hear the opinions from other reviewers: @fhahn @jaykang10 @chill


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

https://reviews.llvm.org/D106426