[PATCH] D136180: [FuncSpec] Compute specialisation gain even when forcing specialisation

[Momchil Velikov via Phabricator via llvm-commits]

chill added a comment.

In D136180#3867123 <https://reviews.llvm.org/D136180#3867123>, @ChuanqiXu wrote:

> The patch itself look good to me. And it looks like the fundamental problem is that we'll match different specializations with a single callsties. Although we workaround it, it'll be better solve the root problems.

I don't think it's a problem at all, it's just what is, for a call site with `n` constants, there are `2^n-1` possible specialisations, that match the call.
They would have different cost/benefit and we will choose one with the biggest gain.

The cost function **must** be such that if we have two specialisations

  s_0(c_0, c_1, ..., c_n, a_0, a_1, ..., a_m)

and

  s_1(d_0, d_1, .., d_k, b_0, b_1, .., b_p)

(where `n + m == k + p`, `c_i, d_i` - constants, `a_i, b_i` - formal parameters, parameters reordered without loss of generality)

such  that

  {c_0, c_1, ..., c_n} < {d_0, d_1, ..., d_m}

then `s_1` has a better cost than  `s_0`.

That leaves as with specialisations over disjoint sets of constants, and, indeed, some of these may have equal gain, but then there is no reason to prefer one over the other, so we choose the one that deterministically comes first.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:456
     // Remove unprofitable specializations.
-    Specializations.remove_if(
-        [](const auto &Entry) { return Entry.second.Gain <= 0; });
+    if (!ForceFunctionSpecialization)
+      Specializations.remove_if(
----------------
ChuanqiXu wrote:
> labrinea wrote:
> > I can see that you are not removing unprofitable specializations, which means you've preserved the "force" semantics of the command line option. Have you checked whether any of the existing tests breaks with this change?
> It looks like we can preserve the current semantics since we don't check the Gains further. It is a little bit odd if we'll specialize the callosities of which has Gain less than 0. But the author explains "assumption that this flag has purely debugging
> purpose and it is reasonable to ignore the extra computing effort it incurs." And this explanation looks reasonable to me.
> Have you checked whether any of the existing tests breaks with this change?

No test breaks (which might mean there is lack of test coverage). 


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

https://reviews.llvm.org/D136180