[PATCH] D126455: [FuncSpec] Make the Function Specializer part of the IPSCCP pass.

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:671-680
+
+  for (unsigned I = 0; I < Clones.size(); ++I) {
+    Function *Clone = Clones[I];
+    CallSpecBinding &Specialization = Specializations[I];
 
-    // Replace some unresolved constant arguments.
-    constantArgPropagation(FuncDecls, M, Solver);
+    LLVM_DEBUG(dbgs() << "FnSpecialization: Replacing call sites of "
+                      << F->getName() << " with " << Clone->getName() << "\n");
----------------
chill wrote:
> chill wrote:
> > You can swap the order of the loops and get rid of `CallSiteToRewrite`.
> Maybe I'm getting a bit ahead of me, but you can change the function to rewrite just a single call site and factor out the iteration over call sites.  The benefit is the function becomes more reusable in as you can independently choose the set of call sites it operates upon. (Incidently, I'm planning to use it that way, but it generally a good change, even if what I have in mind  turns out non-working).
You mean to traverse F's users here instead? We are iterating over them while modifying them, which is the reason why CallSiteToRewrite existed in the first place I believe. Also the dynamic cast and other checks we do: CS->getCalledFunction() == F && Solver.isBlockExecutable(CS->getParent()) (note: this one is missing from the current revision) don't need to be repeated on every iteration of the outer loop which walks the specializaions.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:671-680
+
+  for (unsigned I = 0; I < Clones.size(); ++I) {
+    Function *Clone = Clones[I];
+    CallSpecBinding &Specialization = Specializations[I];
 
-    // Replace some unresolved constant arguments.
-    constantArgPropagation(FuncDecls, M, Solver);
+    LLVM_DEBUG(dbgs() << "FnSpecialization: Replacing call sites of "
+                      << F->getName() << " with " << Clone->getName() << "\n");
----------------
labrinea wrote:
> chill wrote:
> > chill wrote:
> > > You can swap the order of the loops and get rid of `CallSiteToRewrite`.
> > Maybe I'm getting a bit ahead of me, but you can change the function to rewrite just a single call site and factor out the iteration over call sites.  The benefit is the function becomes more reusable in as you can independently choose the set of call sites it operates upon. (Incidently, I'm planning to use it that way, but it generally a good change, even if what I have in mind  turns out non-working).
> You mean to traverse F's users here instead? We are iterating over them while modifying them, which is the reason why CallSiteToRewrite existed in the first place I believe. Also the dynamic cast and other checks we do: CS->getCalledFunction() == F && Solver.isBlockExecutable(CS->getParent()) (note: this one is missing from the current revision) don't need to be repeated on every iteration of the outer loop which walks the specializaions.
If I change it the way you suggest it will regress the current behaviour. Qsort() from SPEC's mcf won't specialize (it's a recursive function) and it's been quite a drive for this work. What if you alter it once you refactor how we rewrite callsites?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126455