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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:140
 using FuncList = SmallVectorImpl<Function *>;
-using ConstList = SmallVector<Constant *>;
-using SpecializationList = SmallVector<SpecializationInfo>;
+using ConstList = SmallVector<std::pair<CallBase *, Constant *>>;
+using SpecializationMap = SmallMapVector<CallBase *, SpecializationInfo, 8>;
----------------
It it worth to give a new name. The current name looks not straight.


================
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);
----------------
labrinea wrote:
> 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.
It should be possible to implement if we would like to add some new local variables. There might be some extra copies. But one the one hand, we get better readability. On the other hand, the extra copies might be significant I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880