[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: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>;
----------------
ChuanqiXu wrote:
> It it worth to give a new name. The current name looks not straight.
I can't think of a better name. Any ideas?


================
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);
----------------
ChuanqiXu wrote:
> 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.
I just read the implementation of `erase()` in MapVector and it's linear to the number of entries :( However it should be possible to use `remove_if()` instead, which is also linear, but it can erase multiple elements in a single pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880