[PATCH] D135463: [FuncSpec] Do not generate multiple copies for identical specializations.

[Momchil Velikov via Phabricator via llvm-commits]

chill added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:526
+      // Create a copy of the function if it doesn't already exist.
+      hash_code Key = hash_value((ArrayRef<ArgInfo>)Info.Args);
+      auto Pair = Clones.insert({Key, nullptr});
----------------
Avoid C-style casts. It should work as `hash_value(ArrayRef<ArgInfo>(Info.Args));`


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:528
+      auto Pair = Clones.insert({Key, nullptr});
+      Function *&Clone = Pair.first->second;
+      if (Pair.second) {
----------------
This is not correct, if the hash values clash two different specialisation will be regarded as the same and one will be discarded.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:529
+      Function *&Clone = Pair.first->second;
+      if (Pair.second) {
+        ValueToValueMapTy Mappings;
----------------
Discarding duplicates should happen before we decide which of the possible specialisations to perform. For example, in `calculateGains` the `MaxClonedThreshold` limit is applied on a list, which potentially contains duplicates.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:534
+        // formal arguments of the specialization.
+        for (User *U : F->users())
+          if (auto CS = dyn_cast<CallBase>(U))
----------------
This looks like a big redundancy, we traverse all the call sites of `F` for each specialisation while all we need is traverse all the call sites of `F` once and redirect the call site to the correct specialisation.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:539
+                    unsigned ArgNo = Arg.Formal->getArgNo();
+                    return CS->getArgOperand(ArgNo) == Mappings[Arg.Formal];
+                }))
----------------
So the idea here is, when specializing on the second parameter only, to turn
```
void g(int x, int y) {
...
  g(x, y);
...
}
...
g(1, 2);
```
into

```
void g.1(int x, /* unused */ int y) {
...
  g.1(x, 2);
...
}
...
g.1(1, 2);
```
But the same ought to happen for:
```
void g(int x, int y) {
...
  g(x, 2);
...
}
...
g(1, 2);
```
and it looks to me the test will miss it because it will compare `2` (from the call argument) to `y` in the cloned function.

(Similar issue in the original code as well).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135463