[PATCH] D139346: [FuncSpec] Global ranking of specialisations

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea accepted this revision.
labrinea added a comment.

Compilation times look way better now, thanks!

My measurements for llvm testsuite with [NewPM-O3 + LTO + FuncSpec=ON] of single threaded [user + system] time spent on IPSCCP show:

| **testname**     | **Delta %** |
| mafft            | -0.397      |
| Bullet           | +0.075      |
| consumer-typeset | -1.931      |
| SPASS            | -0.212      |
| kimwitu++        | -0.891      |
| ClamAV           | -0.694      |
| sqlite3          | -0.320      |
| lencod           | +0.527      |
| 7zip             | +0.063      |
| tramp3d-v4       | -0.470      |
| geomean          | -0.427      |
|

(measured best three runs by passing the `-time-passes` option to lld)

In term of Instruction Count of the total compilation (not of IPSCCP only) I am seeing:

| **testname**     | **Delta %** |
| ClamAV           | +0.082      |
| 7zip             | -0.023      |
| tramp3d-v4       | -0.019      |
| kimwitu++        | -0.010      |
| sqlite3          | -0.040      |
| mafft            | -0.013      |
| lencod           | -0.006      |
| SPASS            | -0.004      |
| consumer-typeset | +0.017      |
| Bullet           | -0.020      |
| geomean          | -0.004      |
|



In D139346#3981106 <https://reviews.llvm.org/D139346#3981106>, @chill wrote:

> In D139346#3980918 <https://reviews.llvm.org/D139346#3980918>, @labrinea wrote:
>
>> All 12 specializations are coming from get_mem_mv, but only MaxClonesThreshold ought to be kept.
>
> Well, yes and no.
>
> For the "no" part:
> We may well create more than `MaxClonesThreshold` specialisations for a single function, as long as the total number of specializations across all functions does not exceed `NumCandidates * MaxClonesThreshold` where `NumCandidates` is the number of functions with at least one specialisation.
>
> For the "yes" part:
> There was a bug computing `NumCandidates` in which resulted in the pass doing more specialisations than allowed in this test.

Apologiers for not being clear. That's what I meant: `NumCandidates=1` ( function get_mem_mv() ) for **lencod** and so we should be keeping `(NumCandidates=1) x (MaxClonesThreshold=3)`. Thanks for fixing the issue with `NumCandidates`  :)



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:237-244
+hash_code llvm::hash_value(const ArgInfo &A) {
+  return hash_combine(hash_value(A.Formal), hash_value(A.Actual));
+}
+
+hash_code llvm::hash_value(const SpecSig &S) {
+  return hash_combine(hash_value(S.Key),
+                      hash_combine_range(S.Args.begin(), S.Args.end()));
----------------
Can we move these in the corresponding header files, where they are declared as friends? I am seeing a warning when I compile:
> warning: ‘llvm::hash_code llvm::hash_value(const llvm::ArgInfo&)’ has not been declared within ‘llvm’
> warning: ‘llvm::hash_code llvm::hash_value(const llvm::SpecSig&)’ has not been declared within ‘llvm’




================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:246-258
+template <> struct llvm::DenseMapInfo<SpecSig> {
+  static inline SpecSig getEmptyKey() { return {~0U, {}}; }
+
+  static inline SpecSig getTombstoneKey() { return {~1U, {}}; }
+
+  static unsigned getHashValue(const SpecSig &S) {
+    return static_cast<unsigned>(hash_value(S));
----------------
Should we maybe move this one too inside `FunctionSpecialization.h` ?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:462
     }
-    Added = false;
-  }
-
-  // Remove unprofitable specializations.
-  if (!ForceFunctionSpecialization)
-    Specializations.remove_if(
-        [](const auto &Entry) { return Entry.second.Gain <= 0; });
-
-  // Clear the MapVector and return the underlying vector.
-  WorkList = Specializations.takeVector();
+    
+    if (S.Args.empty())
----------------
Please remove whitespace.


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

https://reviews.llvm.org/D139346