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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea marked 4 inline comments as done.
labrinea added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h:69
+};
+
+using CallSpecBinding = std::pair<CallBase *, SpecializationInfo>;
----------------
I've removed an unused typedef from here ;)


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:212-213
       // Add the changed CallInst to Solver Worklist
       if (Changed)
         Solver.visitCall(*Call);
     }
----------------
I am not sure whether this is necessary. The unit tests which exercise recursion are passing without it at least.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:275
+    Solver.solveWhileResolvedUndefsIn(Clones);
+    rewriteCallSites(&F, Clones, Specializations);
   }
----------------
We now call this function once, not for every clone as it used to be.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:666-670
+  SmallVector<CallBase *, 8> CallSitesToRewrite;
+  for (User *U : F->users())
+    if (auto *CS = dyn_cast<CallBase>(U))
+      if (CS->getCalledFunction() == F)
+        CallSitesToRewrite.push_back(CS);
----------------
There might be more call sites to rewrite than those in the CallSpecBinding that we have already found, therefore we need to repeat this look up of users here, but at least it now happens once for F compared to Clones.size() times which was the case before.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:682
+      CallBase *CS = *I;
+      if (CS == Specialization.first ||
+          all_of(Specialization.second.Args, [CS, this](const ArgInfo &Arg) {
----------------
no need to examine lattices of arguments if it's the key of the CallSpecBinding


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:689-690
+        CS->setCalledFunction(Clone);
+        std::swap(*I--, *--IE);
+        CallSitesToRewrite.pop_back();
+      }
----------------
We are modifying the list whist traversing it, so we swap the current element with the last one and reduce the iteration range by one.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:700
+        return isa<Instruction>(U) &&
+               cast<Instruction>(U)->getFunction() == F; })) {
+    Solver.markFunctionUnreachable(F);
----------------
the condition was different before, but I think this is correct


================
Comment at: llvm/lib/Transforms/Utils/SCCPSolver.cpp:465-483
+  void solveWhileResolvedUndefsIn(Module &M) {
+    bool ResolvedUndefs = true;
+    while (ResolvedUndefs) {
+      solve();
+      ResolvedUndefs = false;
+      for (Function &F : M)
+        ResolvedUndefs |= resolvedUndefsIn(F);
----------------
I couldn't template these two. The main reason was that one iterates over `Function *` whereas the other over `Function &`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126455