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

[Momchil Velikov via Phabricator via llvm-commits]

chill added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/CMakeLists.txt:97
   InstCombine
+  IPO
   Support
----------------
labrinea wrote:
> chill wrote:
> > Why add IPO here ?
> I vaguely remember a link time error without this change. See also `llvm/utils/gn/secondary/llvm/lib/Transforms/Scalar/BUILD.gn` at the bottom of this diff. The IPSCCP pass now depends on the FunctionSpecializer whose cpp file is under the IPO directory.
IPO already depends on Scalar, i.e. in `IPO/CMakeLists.txt` we  have
```
...

  COMPONENT_NAME
  IPO

  LINK_COMPONENTS
...
  Scalar
...
```

Looks like a circular dependency.

Perhaps `FunctionSpecialization` needs to go to `Utils` (alongside `SCCPSolver`).
Or `runIPSCCP` needs to go  to `IPO/SCCP.cpp`.
Or both.


================
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:
> 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.





================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:237
   Solver.removeLatticeValueFor(&Inst);
-  Inst.eraseFromParent();
+  Inst.removeFromParent();
+  ToDelete.insert(&Inst);
----------------
labrinea wrote:
> chill wrote:
> > Is there a specific reason to remove the instruction from the block?
> > If not, I'd suggest doing deletion in a single place, as opposed to spreading parts of it all over.
> I am not entirely sure. I wanted to avoid revisiting this instruction accidentally in either of simplifyInstsInBlock(), solve(), or resolvedUndefsIn(). For simplifyInstsInBlock() I could skip the instruction if it's present in `ToDelete`. For the others I don't know what the consequences of revisitng would be. I need to run some tests first.
I can't see why would anything go wrong if the instruction is revisited.
Do we know if the instruction is safe to remove? It could be `SDiv`/`SRem` with a zero divisor.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126455