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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Sorry for the delay. I need to look a bit more at this, but I added some first thoughts inline.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:90
 
+static cl::opt<unsigned> MinGainThreshold(
+    "func-specialization-min-gain", cl::Hidden,
----------------
A few comments about this:
- First, thanks for restoring the penalty, I like doing things one step at a time. :)
- Can you add comments what this is, e.g. why it is 10000, but more generally what the rationale is and what this achieves.
- Can you add how this interacts with other options, like MaxClonesThreshold.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:427
+        if (!ForceFunctionSpecialization)
+          S.Gain += getSpecializationBonus(&FormalArg, ActualArg);
+        S.Args.push_back({&FormalArg, ActualArg});
----------------
I probably need to think a bit more about this, but perhaps you can help.... That is, now we are considering multiple arguments, and we calculate the bonus for each of them, and accumulate the bonus here. I am wondering if composition of the bonus is the right model. Alternatives are: take the average, take the minimum/maximum, sum them like you do here, or something else? Do you have any thoughts on this?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:439
+      while (Specializations.size() > MaxClonesThreshold)
+        Specializations.erase(Specializations.begin());
+    } else {
----------------
Can we just do a `return` after this, thus don't need the `else` and decrease indentation? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880