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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

It's looking good to me too, just a last round of nits (inlined) and one question just to double check: is the SPEC score the same? I.e., do we still specialise what we want to specialise?



================
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>;
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:431
+        if (I.second)
+          S.Gain = ForceFunctionSpecialization ? 1 : 0 - Cost;
+        if (!ForceFunctionSpecialization)
----------------
Nit: can `0 - Cost` just be `-Cost`?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:440
+    Specializations.remove_if(
+        [](const auto &Entry) { return Entry.second.Gain <= 0; });
+
----------------
Another nit: if the gain is 0, then arguably it is not a gain and we increase code-size?


================
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
----------------
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.


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

https://reviews.llvm.org/D119880