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

Chuanqi Xu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 2 18:48:39 PDT 2021


ChuanqiXu added a comment.

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

> In D106426#2915448 <https://reviews.llvm.org/D106426#2915448>, @ChuanqiXu wrote:
>
>> 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.
>
> So yes, I think we also agree on this. Let me summarise this then:
>
> - At the moment the number of specialised recursive functions grows linearly by increasing `func-specialization-max-iters`. But that's by design, to support recursive functions.
> - And that obviously comes at a compile-time cost. That's why, realistically, in its current form `func-specialization-max-iters` will never be a big number (say e.g. more than 20).
> - Since compile-time is directly controlled by `func-specialization-max-iters`, that will remain at 1 for the moment. Users can opt-in and pay the cost if they want. Our ambition is to set it something higher, but that depends on the next steps.
>
> Next, and crucially, to address your points:
>
> - A 100% agreed of course: if we can reduce compile times, if we can come up with better cost-modeling, we should do that!
> - I think that will be very easy to add that as a follow up. For example, passing in an iteration number to the bonus/penalty calculation function is trivial. We just need to know what exactly the changes to cost-model should be. I think this data is missing at the moment, see next point.
> - the 10 iterations for the "aggressive mode" was not that a coincidence: this was chosen to get completely rid of a recursive function in exchange from SPEC. When we start looking into recursive functions and increasing the max iterations, we will still want to support that case, and hopefully others too to make it general, and then find a good compile-time trade off.
>
> So yes, I am happy to support you or look into any improvements myself in this area to improve things if we can.

Yeah, now the things get clearer. The overall patch looks good to me. I would try to reduce the time a recursive function get specialized when  `func-specialization-max-iters` increases in follow up patches.



================
Comment at: llvm/lib/Transforms/Utils/SCCPSolver.cpp:1259-1261
+      // Jut bail in case predicate info for ssa.copy is missing.
+      if (!PI)
+        return;
----------------
When would PI be nullptr? It looks like we didn't update the solver in time.


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

https://reviews.llvm.org/D106426



More information about the llvm-commits mailing list