[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/Scalar/CMakeLists.txt:97
   InstCombine
+  IPO
   Support
----------------
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.


================
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);
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:237
   Solver.removeLatticeValueFor(&Inst);
-  Inst.eraseFromParent();
+  Inst.removeFromParent();
+  ToDelete.insert(&Inst);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126455