[PATCH] D119880: [FuncSpec] Support function specialization across multiple arguments.

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

Sorry for the late reply.

>From the implementation, the main change is the introduction of `MinGainThreshold`. It would sort the candidates and erase the late half of candidates if `MinGainThreshold ` is zero, do I understand right? And if `MinGainThreshold ` is not zero, all the candidates whose benefit is below it would be erased. The idea it self looks not bad to me. But I would like to see an upper bound for the number of specialized functions. There isn't one in current version, right? I feel good with the idea: `auto NumSpecKept = (size_t)std::log10(std::pow(Sorted.size(), 4))+1;` or anything else. I am OK to implement this one in following patches. But we need one indeed.

---

Beyond this patch, I think the main concern is still about the cost model. All of us talked about in the revision is about a high level. We are talking about how to filter the candidates. But I think the key point or the problem might be the cost model it self. I mean, why the benefit or cost gets the number for a particular function and correspond constant argument. The original model is relatively simple and I think it has a large space to improve.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:122
+// Disabled by default as it can significantly increase compilation times.
+// Running nikic's compile time tracker on x86 with instruction count as the
+// metric shows 3% regression for SPASS while being neutral for all other
----------------
We should add a link to Nikic's compile time tracker. I guess there might be readers who don't know it.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:314-315
     bool Changed = false;
+    SmallDenseMap<Function *, SpecializationMap> Map;
+    std::map<InstructionCost, CallBase *> Sorted;
+
----------------
I agree that the choice of data structure here is not clean enough. From the implementation, I feel `Map` could be a sorted heap (or PriorityQueue). Here are my thoughts.
```
llvm::PriorityQueue<std::pair<Function *, SpecializationMap>> Map; // We need a self-defined compare class here.
                                                                                                                     // Maybe we need a new name.
```

And we could remove `Sorted`.

Then we could insert the valid pair to `Map`. Then we could keep it sorted. Then we could only pop the later half candidates. 

It is ok to use vector like container and use `heap` APIs in std to build the heap by hand. The benefit could be that we could access the container randomly. (So we could avoid pop iterators one by one)


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:331
 
-      SpecializationList Specializations;
+      SpecializationMap &Specializations = Map[F];
       calculateGains(F, Cost, Specializations);
----------------
We shouldn't insert `F` to `Map` directly. Since it is possible that this Specializations is not valid.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:501-507
+        S.Gain -= Cost;
+        if (S.Gain < MinGainThreshold)
+          KeysToRemove.push_back(Call);
       }
+      // Remove unprofitable specializations.
+      for (CallBase *Call : KeysToRemove)
+        Specializations.erase(Call);
----------------
The 2 stage construction of `Specializations` looks not good. It would fill things in Specializations first and remove the unwanted things. It is unnecessarily wasteful. We should check before inserting. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880