chill added inline comments.
================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:239
+/// \returns true if at least one function is specialized.
+bool FunctionSpecializer::specialize(SmallVectorImpl<Function *> &WorkList) {
+ for (Function &F : M) {
----------------
`WorkList` is a too generic name for a parameter and it's not a worklist per se anyway.
================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:275
+ NumFuncSpecialized += NbFunctionsSpecialized;
+ return !WorkList.empty();
+}
----------------
This is a little bit fragile in the sense the caller may forget to clear the list.
It would be nicer if this function itself clears the list first thing when it starts execution.
`WorkList` could also be made a return, taking advantage of move semantics, which looks nicer on paper, but
may cause a few allocations/deallocations if we iterate.
================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:116
+static void replaceUsesOfWith(SCCPSolver &Solver, Value *Old, Value *New) {
+ SmallVector<User *> Users(Old->users());
+ Old->replaceAllUsesWith(New);
----------------
Wouldn't it work without the temporary vector?
`markUsersAsChanged` would go over each user, look at the user's operands (including `Old`),
and find the `New` (which is some constant) form the lattice values map.
Thus we would maybe get just:
```
Solver.markUsersAsChanged(Old);
Old->replaceAllUsesWith(New);
```
================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:635
+
+ for (Function &F : M)
+ WorkList.push_back(&F);
----------------
I would suggest not creating a vector of all the functions in the module as they could be quite a lot (e.g. in LTO)
and thus trigger several heap allocations for `WorkList`.
`solveWhileResolvedUndefIn` is quite small and could be overloaded for a `Module *` parameter.
I considered making this function a template along the lines of:
```
template<typename RangeT>
void printNames(RangeT &&R) {
for (auto &F : R)
llvm::dbgs() << magic(F)->getName();
}
std::vector<llvm::Function *> v;
llvm::Module *M;
int main() {
printNames(M->functions());
printNames(v);
}
```
but couldn't come up with `magic`.
As for `propagateConstants` it could be done with a few overloads as well:
```
static bool propagateConstants(SCCPSolver &Solver, Function *F,
SmallPtrSetImpl<Instruction *> &ToDelete);
static bool propagateConstants(SCCPSolver &Solver,
SmallVectorImpl<Function *> &WorkList,
SmallPtrSetImpl<Instruction *> &ToDelete) {
for (Function *F : WorkList)
propagateConstants(Solve, F, ToDelete);
}
static bool propagateConstants(SCCPSolver &Solver, Module *M,
SmallPtrSetImpl<Instruction *> &ToDelete) {
for (auto &F : Module)
propagateConstants(Solve, &F, ToDelete);
}
```
================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:161-172
+ // Record uses of V to avoid visiting irrelevant uses of const later.
+ SmallVector<Instruction *> UseInsts;
+ for (auto *U : V->users())
+ if (auto *I = dyn_cast<Instruction>(U))
+ if (Solver.isBlockExecutable(I->getParent()))
+ UseInsts.push_back(I);
+
----------------
labrinea wrote:
> chill wrote:
> > labrinea wrote:
> > > labrinea wrote:
> > > > chill wrote:
> > > > > labrinea wrote:
> > > > > > Just found that we need to do the same inside `replaceSignedInst()` too. I will move this code a function.
> > > > > Would it be possible to call `markUsersAsChanged` here ?
> > > > I think we can't because if we replace the uses first then the users of the old value will be empty. Can we markUsersAsChanged before we replaceAllUsesWith the new value? Btw markUsersAsChanged is private for the SCCPInstVisitor, but I suppose I could make it public if need be.
> > > Actually I could call markUsersAsChanged on the new Instrcution after replacing the uses of the old Instruction with it.
> > OK, let's leave it hanging for now, until I can take a look on top of the latest trunk.
> >
> > Ideally, we are trying to avoid changing code until the Solver is done.
> > Here we have found that an instruction has constant lattice value - we should not replace the users' operands
> > right away, but notify the Solver. The Solver in turn would add the instructions that need reexamining to the instructions worklist and update their lattice values the next time we invoke `Solvet.solve()`.
> >
> > Most likely `SCCPSolver::visit` should become private, the Solver (and the SCCP algorithm in general) is
> > driven by its worklists, we should stick to this design: want something done - add it to the worklist.
> >
> >
> >
> Update: I tried this. It works for 'some' cases. Instead of replacing values with constants I create mappings from the old to the new value and only after all the solving is done then I replace the uses. The specialization of recursive functions doesn't work because it relies on finding allocas of constant integers. Also the rewriting of callsites doesn't work either if the actual arguments have been constant propagated prior to specialization, but the old value hasn't been replaced yet. In theory I could pass on the mappings from sccp to the specializer but it seems overly complicated to do so.
> Instead of replacing values with constants I create mappings from the old to the new value ..
But isn't this what the `ValueState` already contains?
> Also the rewriting of callsites doesn't work either if the actual arguments have been constant propagated prior to specialization, but the old value hasn't been replaced yet.
Well, `FunctionSpecializer::rewriteCallSites` and everything else should lookup lattice values, not work directly with operands.
But OK, let's not make too many changes at once and revisit it later.
================
Comment at: llvm/lib/Transforms/Utils/SCCPSolver.cpp:1577
+void
+SCCPSolver::solveWhileResolvedUndefsIn(SmallVectorImpl<Function *> &WorkList) {
----------------
All the functions here forward to the `Visitor`, this one should also just be forwarding.
(Not sure why this proxy class exists at all, but I guess we can address it later).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126455/new/
https://reviews.llvm.org/D126455