[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: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();
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > Unnecessary change.
> Clang format made this.
We could format only for diffs. This is what we generally do.


================
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;
+    });
----------------
labrinea wrote:
> 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.
To get D121817 accepted, I think you need to comment on that thread. I think there is no block comments now. For the problem of sorting MapVector, I think add a member function `sort()` to MapVector is accepted solution. In my imagination, I feel it wouldn't be hard to implement.

But from the context of the current patch, I found we don't need to use MapVector at all. The first part of the pair of the map didn't get used at all. I guess this is due to the patch get split. So I think we could not solve the problem for MapVector now. We could solve it after we need.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880