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


================
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:
> 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.
As I have explained on the description we do need an associative container to bind all the actual arguments of a specialization to the same function call. That is only needed during the gain calculation phase. After that we only care about iterating over the specializations found. Therefore, I don't see the need to update the map indices while sorting the vector. It is expensive and in my opinion unnecessary.

> 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 am not following. Could you please explain?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880