[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:308
         LLVM_DEBUG(
-            dbgs() << "FnSpecialization: Invalid specialisation cost.\n");
+            dbgs() << "FnSpecialization: Invalid specialization cost.\n");
         continue;
----------------
We could submit such changes standalone as a NFC patch.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:328
+
+    return NbFunctionsSpecialized > 0;
   }
----------------
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.


================
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();
----------------
Unnecessary change.


================
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;
+    });
----------------
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`?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880