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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

In D104365#2843823 <https://reviews.llvm.org/D104365#2843823>, @ChuanqiXu wrote:

> Add an option to control the behavior.
>
> Then I did some statistics for SPEC2017int
>
> The total number of specialized functions increased about 18% (from 49 to 58)
>
> Then I didn't find significant compile-time change with turning option on.
>
> I wonder if this means that we can turn this option on by default.

This looks good to me. Let's start with having this off by default. Like I mentioned earlier, for this to be enabled too by default we probably need some more numbers than only SPEC, but also e.g. a bootstrap build, llvm test suite, sqlite, etc. But now that we have this option, we can experiment with this, get more numbers, and also address some other TODOs before we try to enable function specialisation by default.

Before committing, can you rephrase the subject "Enabling specializing direct constant", make the description a bit more descriptive, and look at the inlined nits.

> The total number of specialized functions increased about 18% (from 49 to 58)

I am surprised by this big number of specialisations. I remember seeing only 4 specialisations for SPEC2017int.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:15
 // - It does not handle specialization of recursive functions,
 // - It does not yet handle integer constants, and integer ranges,
 // - Only 1 argument per function is specialised,
----------------
Nit: we can remove the "integer constants", but keep the integer ranges.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:67
 
+static cl::opt<bool> EnableSpecializeForLiteralConstant(
+  "function-specialization-for-literal-constant", cl::init(false), cl::Hidden,
----------------
Nit: perhaps `EnableSpecializeForLiteralConstant` -> `EnableSpecializationForLiteralConstant`


================
Comment at: llvm/test/Transforms/FunctionSpecialization/function-specialization-constant-integers.ll:3
+
+; Check that the direct constant parameter could be specialized.
+; CHECK: @foo.1(
----------------
Nit: perhaps `direct constant` -> `literal constant`


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

https://reviews.llvm.org/D104365