[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:124
 using FuncList = SmallVectorImpl<Function *>;
-using ConstList = SmallVector<Constant *>;
-using SpecializationList = SmallVector<SpecializationInfo>;
+using ActualArgPair = std::pair<CallBase *, Constant *>;
+using ActualArgPairVector = SmallVector<ActualArgPair, 8>;
----------------
SjoerdMeijer wrote:
> Sorry, bikeshedding names: if we group the caller (CallBase) and a constant actual argument, is `ActualArgPair` an accurate description? We don't group 2 actual args, which is what this name is suggesting to me.
I agree it's not a good type name. Any ideas? Maybe it's not really necessary do define one.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:431
+        if (I.second)
+          S.Gain = ForceFunctionSpecialization ? 1 : 0 - Cost;
+        if (!ForceFunctionSpecialization)
----------------
SjoerdMeijer wrote:
> Nit: can `0 - Cost` just be `-Cost`?
We can't because the InstructionCost class does not implement the `-` operator.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:440
+    Specializations.remove_if(
+        [](const auto &Entry) { return Entry.second.Gain <= 0; });
+
----------------
SjoerdMeijer wrote:
> Another nit: if the gain is 0, then arguably it is not a gain and we increase code-size?
We are doing the same as before (see line 423 on the left) : we reject specializations with Gain that is less or equal to zero. How does this increase code size?


================
Comment at: llvm/lib/Transforms/Utils/SCCPSolver.cpp:542
 
-    if (OldArg == Arg.Formal) {
+    if (OldArg == (*Iter).Formal) {
       // Mark the argument constants in the new function.
----------------
ChuanqiXu wrote:
> Could we use `Iter->Formal`?
possibly


================
Comment at: llvm/test/Transforms/FunctionSpecialization/specialize-multiple-arguments.ll:8
+; Make sure that we iterate correctly after sorting the specializations:
+; FnSpecialization: Specializations for function compute
+; FnSpecialization:   Gain = 608
----------------
SjoerdMeijer wrote:
> Perhaps you want to turn this into a test, i.e. match the debug output too? If so, that would require asserts. Not sure, but probably best done as an additional, separate test, otherwise we might loose the testing of this for the non-assert builds and bots.
This here is a comment; it does not contain check lines. The whole purpose is to show that specializations do get sorted and iterated correctly.

I wouldn't mind adding test for debug output in a follow up patch. I couldn't find examples; not sure how to do them. Any pointers appreciated.


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

https://reviews.llvm.org/D119880