[PATCH] D107897: [FuncSpec] Don't specialize function which are easy to inline

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added subscribers: LilyZhong, apostolakis.
snehasish added a comment.

> - Function Specialization could be valuable without PGO.
> - With PGO, the effect of function specialization may be questionable. But we need to experiment it more.
>
> I think the topic of PGO and function specialization is much beyond the scope of this patch.  It is a good question and it may be better to discuss it in llvm-dev.

Agreed, since the focus is currently on plain builds we can continue this approach. In the future, we can share some information on impact with PGO. 
@apostolakis @LilyZhong

> Let me sure if I understand your words correctly. It looks like that you are asking: with function specialization, how the compile-time, code-sizes and performance changes?
> There are results in `D93838`. Simply, the compile-time and code-size change is not aggressive. And the performance change mainly good for 500.mcf_r by 10%.

I was asking if there is a broader characterization <https://reviews.llvm.org/D36432#:~:text=Benchmark,%23%20Specialized%20Funcs> for this work like the 2017 patch contained.

In D107897#2943236 <https://reviews.llvm.org/D107897#2943236>, @SjoerdMeijer wrote:

> As explained, there's a reason for the current pipeline organisation, and this minor addition, this one-liner to check `AlwaysInline` makes sense in this context. This allows us to get a better baseline, i.e. an function specialisation version for which we can measure compile-time increases compared to clang with function specialisation, and GCC which has this enabled by default. This is my short/mid term objective: see if we can get a first version enabled by default to reach parity with GCC.

Sounds good to me, we can refine the PGO aspects if we find some opportunities.
Additionally, we would be happy to evaluate the impact (compile time, code size) on some large Google workloads once this pass is more mature.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:70
+static cl::opt<unsigned> SmallFunctionThreshold(
+    "func-spec-small-function-threshold", cl::Hidden,
+    cl::desc("For functions whose lines of code below this threshold, "
----------------
SjoerdMeijer wrote:
> Nit: don't think "small" adds anything as it it is just the function threshold. Also the naming is inconsistent with other options (there is already some, but no need to make it worse), so can suggest:
> 
>   -func-specialization-function-size-threshold
> 
> but because that's a bit verbose, perhaps shorter and still descriptive is:
> 
>   -func-specialization-size-threshold
+1 on consistent naming.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:71
+    "func-spec-small-function-threshold", cl::Hidden,
+    cl::desc("For functions whose lines of code below this threshold, "
+             " they wouldn't be specialized to avoid redundant sepcializations."),
----------------
I think number of insts != lines of code where I assume `Metrics.NumInsts` enumerates the IR instruction count. Perhaps rephrase to make it more accurate?


================
Comment at: llvm/test/Transforms/FunctionSpecialization/function-specialization-always-inline.ll:3-4
+
+; CHECK-NOT: foo.1
+; CHECK-NOT: foo.2
+
----------------
Perhaps this can be collapsed into a single `CHECK-NOT: foo.{{[0-9]+}}`


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

https://reviews.llvm.org/D107897