[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:328
+
+    return NbFunctionsSpecialized > 0;
   }
----------------
ChuanqiXu wrote:
> I think this change is not necessary. And we should keep the original `Changed` variable. Given `specializeFunctions` is called multiple times, `NbFunctionsSpecialized` might be bigger than 0 all the time. So the return value of specializeFunctions wouldn't be right.
Good point, I forgot `specializeFunctions` is being called inside a loop without reseting `NbFunctionsSpecialized` first. I'll fix this.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:333-334
     for (auto *I : ReplacedWithConstant) {
-      LLVM_DEBUG(dbgs() << "FnSpecialization: Removing dead instruction "
-                        << *I << "\n");
+      LLVM_DEBUG(dbgs() << "FnSpecialization: Removing dead instruction " << *I
+                        << "\n");
       I->eraseFromParent();
----------------
ChuanqiXu wrote:
> Unnecessary change.
Clang format made this.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:441-446
+    WorkList = Specializations.takeVector();
+
+    // Sort the candidates in descending order.
+    llvm::stable_sort(WorkList, [](const auto &L, const auto &R) {
+      return L.second.Gain > R.second.Gain;
+    });
----------------
ChuanqiXu wrote:
> Could we sort directly on MapVector? I see MapVector implement both `swap` and `operator[]`. So it looks possible to sort directly for MapVector.
> 
> Then could we remove `WorkList`?
We could directly sort on MapVector, yes, but it's the same thing. I think we shouldn't get rid of the `Worklist` because if we use the MapVector after this point the indices are outdated. Therefore if we try to use the [] operator at any point, the value it returns will be wrong. This is hard to observe and we could end up dealing with issues that go unnoticed.

Also we won't be able to erase the excess specializations at the end of the list. If https://reviews.llvm.org/D121817 gets accepted we might be able to do so, but still doing it directly on a vector is cheaper.


================
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:
> labrinea wrote:
> > 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?
> Oh, I am not good at naming too. But I am sure the name is relatively bad. For example, the original `ConstList` is `SmallVector<Constant*>`, which is pretty makes sense. But now it is `SmallVector<std::pair<CallBase *, Constant *>>` and the name is still `ConstList`... It has the same problem with `SpecializationList` and `SpecializationMap`...
> 
> @SjoerdMeijer do you have any suggestion?
Side note: It would be nice if I could at least define `SpecializationList` (or whatever we decide to call it) as SpecializationMap::VectorType, but unfortunately VectorType is private for MapVector.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880