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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D104365#2829836 <https://reviews.llvm.org/D104365#2829836>, @SjoerdMeijer wrote:

> Let me explain why I am a bit pushing back at this: I would like to see function specialisation enabled by default one day,。

Yeah, that's my wish too.

> I don't think we can draw this conclusion because we know that for SPEC function pointers are important. To understand what is happening, I would like to know how many functions were specialised on integer constants. Only then we can say something about this. And in order to get these numbers, it is probably convenient:
> to split the statistics up and have a counter for the constant globals and another for the constant integers, have command line options to toggle on/off this different behaviour. Regardless whether this triggers or not in SPEC, I also think we need a few more data points to draw any conclusions. Because it is easy and relevant, I would suggest at least a bootstrap build and sqlite.

I understand that it is always better to be careful and require an option to control the behavior. I could add an option and do more stats. But I think that it is a little bit counter-intuitive that function specialization wouldn't specialize integer constants. For a general example:

  void foo(..., bool cond) {
      if (cond) {
  
      } else {
  
      }
  }

>From my mind, every one should  think `cond` could be handled by function specialization. My point is that function specialization should support for constant integers. Even we add options, the ability to handle constant integers should be the default behavior too.
Of course, I would do other stats to support it.

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. 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.


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

https://reviews.llvm.org/D104365