[PATCH] D104365: [FuncSpec] Enabling specializing direct constant

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Specialising on constant integers would definitely be good, and is indeed what we want.  But it's early days for function specialisation, and there are quite a few limitations, and this is just one of them.

I disagree with this:

> But even the stats shows that this may incur some CT increasement, I would still think function specialization pass should handle the constant integers. Since it should be.

First of all, because we first need to know what the increase is. Second, when we know the increase, only then we can say if it should or not.

> On the other hand, I think the main part we should focus on is the cost model. In other words, even the CT increases due to this patch, we should investigate on the cost model instead of turn this off simply. I hope that we can get a consensus on this.

Agreed on this. At the moment, we lack insights and data what this patch is doing. If we are going to build on top function specialisation and add more features, I would like to see the the new features and the cost-model to be developed hand in hand, otherwise it is only going to be harder later.

Repeating what I said earlier, I think the order of events should be this: I would like to see more fine-grain stats and options, compile time data for a few different code bases, and then a discussion on the cost-model before we add this.

We would like to achieve parity with GCC which has this enabled by default and is the reason I would like to get this enabled by default. My preferred approach would be to try this for constant globals and function pointers, because if we don't succeed in that, we are definitely not going to succeed if we also specialise on constant integers. Thus, I would like to do this step by step, by address some existing TODOs first to get the code in shape for this enabling this by default, then flip a switch to also specialise on constant integers. Thus, if you do this under an option that is off by default first, I have no problems at all with adding thi, while perhaps in the mean time you work on getting numbers and the cost-model.


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

https://reviews.llvm.org/D104365