[PATCH] D115458: [FuncSpec] Decouple Cost and Benefit analysis, to sort candidates.

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

Do we notice the score/code size change in SPEC after this patch?



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:97
+// to the actual transform helper functions.
+struct ArgInfo {
+  Function *Fn;         // The function to perform specialisaiton on.
----------------
For a class used only in a cpp file, it may be better to wrap it into anonymous namespace to make sure the linkage is internal.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:401
+      // Sort the candidates in descending order.
+      std::sort(Worklist.begin(), Worklist.end(), [] (ArgInfo L, ArgInfo R) {
+                                          return L.Gain > R.Gain; } );
----------------
snehasish wrote:
> Use a reference to avoid copies here?
We could use llvm::sort here to use range style sort.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:406-413
+      if (Worklist.size() > MaxConstantsThreshold) {
+        LLVM_DEBUG(dbgs() << "FnSpecialization: number of constants exceed "
+                          << "the maximum number of constants threshold.\n"
+                          << "Truncating worklist to " << MaxConstantsThreshold
+                          << " candidates.\n");
+        Worklist.erase(Worklist.begin() + MaxConstantsThreshold,
+                       Worklist.end());
----------------
A note: this change is not NFC (I'm fine with this change) @snehasish 


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:415
+
+      if (IsPartial || ActualConstArg.size() != Worklist.size())
+        for (auto &ActualArg : Worklist)
----------------
How about `Worklist.size() < ActualConstArg.size()`?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:463
+    Function *Clone = cloneCandidateFunction(AI.Fn);
+    Argument *ClonedArg = Clone->arg_begin() + AI.Arg->getArgNo();
 
----------------
It might be better to:
```
Clone->getArg(AI.Arg->getArgNo);
```


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

https://reviews.llvm.org/D115458