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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
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
----------------
ChuanqiXu wrote:
> We should add a link to Nikic's compile time tracker. I guess there might be readers who don't know it.
Will do.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:134
 struct SpecializationInfo {
-  ArgInfo Arg;          // Stores the {formal,actual} argument pair.
-  InstructionCost Gain; // Profitability: Gain = Bonus - Cost.
-
-  SpecializationInfo(Argument *A, Constant *C, InstructionCost G)
-      : Arg(A, C), Gain(G){};
+  SmallVector<ArgInfo, 8> Args; // Stores the {formal,actual} argument pairs.
+  InstructionCost Gain = 0;     // Profitability: Gain = Bonus - Cost.
----------------
SjoerdMeijer wrote:
> Nit: we might as well drop the 8 here. From the programmers manual:
> 
> > In the absence of a well-motivated choice for the number of inlined elements N, it is recommended to use SmallVector<T> (that is, omitting the N).
I tried to use realistic numbers in all the Small ADTs in this patch. I don't think we'll ever get to a specialization with more than 8 arguments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:314-315
     bool Changed = false;
+    SmallDenseMap<Function *, SpecializationMap> Map;
+    std::map<InstructionCost, CallBase *> Sorted;
+
----------------
ChuanqiXu wrote:
> 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)
Thanks for the input. I have a working revision which does what I suggested in my last comment to Sjoerd - a flattened SpecializationMap that contains entries for multiple functions, which I am then sorting with stable sort. I'll upload it soon.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:331
 
-      SpecializationList Specializations;
+      SpecializationMap &Specializations = Map[F];
       calculateGains(F, Cost, Specializations);
----------------
ChuanqiXu wrote:
> We shouldn't insert `F` to `Map` directly. Since it is possible that this Specializations is not valid.
Good catch, in an improved version of this revision I removed the entry inside the following block `if (Specializations.empty()) {`. But as I said I will upload a different approach.


================
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);
----------------
ChuanqiXu wrote:
> 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. 
I am not sure if that's possible as we only know if the Gain is above the threshold after we've accumulated the bonus from each argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880